Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make token manager the minter for interchain tokens #313

Closed
wants to merge 4 commits into from
Closed

feat: make token manager the minter for interchain tokens #313

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0e5277
stash
Foivos Dec 11, 2024
d48805e
Merge branch 'feat/achee-report-29-11-24' into feat/token-manager-min…
Foivos Dec 11, 2024
befb487
Fix tests
Foivos Dec 11, 2024
a4b3962
Change an error a bit
Foivos Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contracts/InterchainTokenFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M
if (!token.isMinter(minter)) revert NotMinter(minter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add changeset and briefly discuss the behaviour change reason


// Sanity check to prevent accidental use of the current ITS address as the token minter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update DESIGN.md for this behaviour change

if (minter == address(interchainTokenService)) revert InvalidMinter(minter);
if (minter == interchainTokenService.tokenManagerAddress(tokenId)) revert InvalidMinter(minter);
}

/**
Expand Down
15 changes: 3 additions & 12 deletions contracts/TokenHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ pragma solidity ^0.8.0;

import { ITokenHandler } from './interfaces/ITokenHandler.sol';
import { IERC20 } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/interfaces/IERC20.sol';
import { SafeTokenTransfer, SafeTokenTransferFrom, SafeTokenCall } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/libs/SafeTransfer.sol';
import { SafeTokenTransferFrom, SafeTokenCall } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/libs/SafeTransfer.sol';
import { ReentrancyGuard } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/utils/ReentrancyGuard.sol';
import { Create3AddressFixed } from './utils/Create3AddressFixed.sol';

import { ITokenManagerType } from './interfaces/ITokenManagerType.sol';
import { ITokenManager } from './interfaces/ITokenManager.sol';
import { ITokenManagerProxy } from './interfaces/ITokenManagerProxy.sol';
import { IERC20MintableBurnable } from './interfaces/IERC20MintableBurnable.sol';
import { IERC20BurnableFrom } from './interfaces/IERC20BurnableFrom.sol';

/**
Expand Down Expand Up @@ -39,7 +38,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Crea
ITokenManager(tokenManager).addFlowIn(amount);

if (tokenManagerType == uint256(TokenManagerType.NATIVE_INTERCHAIN_TOKEN)) {
_giveInterchainToken(tokenAddress, to, amount);
_mintToken(tokenManager, tokenAddress, to, amount);
return (amount, tokenAddress);
}

Expand Down Expand Up @@ -77,7 +76,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Crea
if (tokenOnly && msg.sender != tokenAddress) revert NotToken(msg.sender, tokenAddress);

if (tokenManagerType == uint256(TokenManagerType.NATIVE_INTERCHAIN_TOKEN)) {
_takeInterchainToken(tokenAddress, from, amount);
_burnToken(tokenManager, tokenAddress, from, amount);
} else if (tokenManagerType == uint256(TokenManagerType.MINT_BURN)) {
_burnToken(tokenManager, tokenAddress, from, amount);
} else if (tokenManagerType == uint256(TokenManagerType.MINT_BURN_FROM)) {
Expand Down Expand Up @@ -160,14 +159,6 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Crea
return diff < amount ? diff : amount;
}

function _giveInterchainToken(address tokenAddress, address to, uint256 amount) internal {
IERC20(tokenAddress).safeCall(abi.encodeWithSelector(IERC20MintableBurnable.mint.selector, to, amount));
}

function _takeInterchainToken(address tokenAddress, address from, uint256 amount) internal {
IERC20(tokenAddress).safeCall(abi.encodeWithSelector(IERC20MintableBurnable.burn.selector, from, amount));
}

function _mintToken(address tokenManager, address tokenAddress, address to, uint256 amount) internal {
ITokenManager(tokenManager).mintToken(tokenAddress, to, amount);
}
Expand Down
3 changes: 2 additions & 1 deletion contracts/interchain-token/InterchainToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
pragma solidity ^0.8.0;

import { IInterchainToken } from '../interfaces/IInterchainToken.sol';
import { IInterchainTokenService } from '../interfaces/IInterchainTokenService.sol';

import { InterchainTokenStandard } from './InterchainTokenStandard.sol';
import { ERC20Permit } from './ERC20Permit.sol';
Expand Down Expand Up @@ -97,7 +98,7 @@ contract InterchainToken is InterchainTokenStandard, ERC20Permit, Minter, IInter
* Also add the provided address as a minter. If `address(0)` was provided,
* add it as a minter to allow anyone to easily check that no custom minter was set.
*/
_addMinter(interchainTokenService_);
_addMinter(IInterchainTokenService(interchainTokenService_).tokenManagerAddress(tokenId_));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like a weird re-entrancy to call ITS back during deployment. better to pass in the token manager, or have ITS call transferMintership after deployment. The latter avoids needing to change the token bytecode, and keeps the logic isolated in ITS

_addMinter(minter);

_setNameHash(tokenName);
Expand Down
7 changes: 3 additions & 4 deletions test/ERC20.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ const {
} = ethers;
const { expect } = require('chai');
const { getRandomBytes32, expectRevert } = require('./utils');
const { deployContract } = require('../scripts/deploy');
const { deployContract, deployAll } = require('../scripts/deploy');

describe('ERC20', () => {
let interchainToken, interchainTokenDeployer;
let interchainTokenDeployer;

const name = 'tokenName';
const symbol = 'tokenSymbol';
Expand All @@ -26,8 +26,7 @@ describe('ERC20', () => {
owner = wallets[0];
user = wallets[1];

interchainToken = await deployContract(owner, 'InterchainToken', [owner.address]);
interchainTokenDeployer = await deployContract(owner, 'InterchainTokenDeployer', [interchainToken.address]);
({ interchainTokenDeployer } = await deployAll(owner, 'Test'));

const salt = getRandomBytes32();
const tokenId = getRandomBytes32();
Expand Down
8 changes: 3 additions & 5 deletions test/ERC20Permit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ const {
} = ethers;
const { expect } = require('chai');
const { getRandomBytes32, expectRevert, getChainId } = require('./utils');
const { deployContract } = require('../scripts/deploy');
const { deployAll } = require('../scripts/deploy');

describe('ERC20 Permit', () => {
let interchainToken, interchainTokenDeployer;
let interchainTokenDeployer;

const name = 'tokenName';
const symbol = 'tokenSymbol';
Expand All @@ -27,9 +27,7 @@ describe('ERC20 Permit', () => {
owner = wallets[0];
user = wallets[1];

interchainToken = await deployContract(owner, 'InterchainToken', [owner.address]);
interchainTokenDeployer = await deployContract(owner, 'InterchainTokenDeployer', [interchainToken.address]);

({ interchainTokenDeployer } = await deployAll(owner, 'Test'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you make the previous suggested change, then you don't need to deploy all contracts for this to work

const salt = getRandomBytes32();
const tokenId = getRandomBytes32();

Expand Down
10 changes: 5 additions & 5 deletions test/InterchainToken.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ const {
} = ethers;
const { expect } = require('chai');
const { getRandomBytes32, expectRevert, getEVMVersion } = require('./utils');
const { deployContract } = require('../scripts/deploy');
const { deployContract, deployAll } = require('../scripts/deploy');

describe('InterchainToken', () => {
let interchainToken, interchainTokenDeployer;
let interchainTokenDeployer;
let interchainToken;

const name = 'tokenName';
const symbol = 'tokenSymbol';
Expand All @@ -28,8 +29,7 @@ describe('InterchainToken', () => {
owner = wallets[0];
user = wallets[1];

interchainToken = await deployContract(owner, 'InterchainToken', [owner.address]);
interchainTokenDeployer = await deployContract(owner, 'InterchainTokenDeployer', [interchainToken.address]);
({ interchainToken, interchainTokenDeployer } = await deployAll(owner, 'Test'));

const salt = getRandomBytes32();
const tokenId = getRandomBytes32();
Expand Down Expand Up @@ -147,7 +147,7 @@ describe('InterchainToken', () => {
const contractBytecodeHash = keccak256(contractBytecode);

const expected = {
london: '0x482146829055f052063003e9cf0ffaf798a12fb58088c2667566a135b9568355',
london: '0x6e99e9cdd22bc7070016c2ca4a88520506fa524a0e91343df4dab44705485991',
}[getEVMVersion()];

expect(contractBytecodeHash).to.be.equal(expected);
Expand Down
10 changes: 5 additions & 5 deletions test/InterchainTokenFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ describe('InterchainTokenFactory', () => {
const checkRoles = async (tokenManager, minter) => {
const token = await getContractAt('InterchainToken', await tokenManager.tokenAddress(), wallet);
expect(await token.isMinter(minter)).to.be.true;
expect(await token.isMinter(service.address)).to.be.true;
expect(await token.isMinter(tokenManager.address)).to.be.true;

expect(await tokenManager.isOperator(minter)).to.be.true;
expect(await tokenManager.isOperator(service.address)).to.be.true;
Expand Down Expand Up @@ -370,7 +370,7 @@ describe('InterchainTokenFactory', () => {
tokenFactory['deployRemoteInterchainToken(string,bytes32,address,string,uint256)'](
'',
salt,
service.address,
tokenManager.address,
destinationChain,
gasValue,
{
Expand All @@ -380,7 +380,7 @@ describe('InterchainTokenFactory', () => {
),
tokenFactory,
'InvalidMinter',
[service.address],
[tokenManager.address],
);

await expect(
Expand Down Expand Up @@ -423,7 +423,7 @@ describe('InterchainTokenFactory', () => {
(gasOptions) =>
tokenFactory['deployRemoteInterchainToken(bytes32,address,string,uint256)'](
salt,
service.address,
tokenManager.address,
destinationChain,
gasValue,
{
Expand All @@ -433,7 +433,7 @@ describe('InterchainTokenFactory', () => {
),
tokenFactory,
'InvalidMinter',
[service.address],
[tokenManager.address],
);

await expect(
Expand Down
2 changes: 1 addition & 1 deletion test/InterchainTokenService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ describe('Interchain Token Service', () => {

const token = await getContractAt('InterchainToken', tokenAddress, wallet);
expect(await token.isMinter(wallet.address)).to.be.true;
expect(await token.isMinter(service.address)).to.be.true;
expect(await token.isMinter(tokenManager.address)).to.be.true;
});

it('Should revert when registering an interchain token as a lock/unlock for a second time', async () => {
Expand Down
17 changes: 8 additions & 9 deletions test/UtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
const chai = require('chai');
const { ethers } = require('hardhat');
const {
Wallet,
getContractAt,
constants: { AddressZero },
} = ethers;
const { time } = require('@nomicfoundation/hardhat-network-helpers');
const { expect } = chai;
const { getRandomBytes32, expectRevert, isHardhat, waitFor } = require('./utils');
const { deployContract } = require('../scripts/deploy');
const { deployContract, deployAll } = require('../scripts/deploy');

let ownerWallet, otherWallet;

Expand Down Expand Up @@ -257,16 +256,15 @@ describe('FlowLimit', async () => {
});

describe('InterchainTokenDeployer', () => {
let interchainToken, interchainTokenDeployer;
const service = new Wallet(getRandomBytes32()).address;
let interchainTokenDeployer;
let service;
const name = 'tokenName';
const symbol = 'tokenSymbol';
const decimals = 18;
const MINTER_ROLE = 0;

before(async () => {
interchainToken = await deployContract(ownerWallet, 'InterchainToken', [service]);
interchainTokenDeployer = await deployContract(ownerWallet, 'InterchainTokenDeployer', [interchainToken.address]);
({ interchainTokenDeployer, service } = await deployAll(ownerWallet, 'Test'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

});

it('Should revert on deployment with invalid implementation address', async () => {
Expand All @@ -283,20 +281,21 @@ describe('InterchainTokenDeployer', () => {
const tokenAddress = await interchainTokenDeployer.deployedAddress(salt);

const token = await getContractAt('InterchainToken', tokenAddress, ownerWallet);
const tokenManager = await service.tokenManagerAddress(tokenId);

await expect(interchainTokenDeployer.deployInterchainToken(salt, tokenId, ownerWallet.address, name, symbol, decimals))
.to.emit(token, 'RolesAdded')
.withArgs(service, 1 << MINTER_ROLE)
.withArgs(tokenManager, 1 << MINTER_ROLE)
.and.to.emit(token, 'RolesAdded')
.withArgs(ownerWallet.address, 1 << MINTER_ROLE);

expect(await token.name()).to.equal(name);
expect(await token.symbol()).to.equal(symbol);
expect(await token.decimals()).to.equal(decimals);
expect(await token.hasRole(service, MINTER_ROLE)).to.be.true;
expect(await token.hasRole(tokenManager, MINTER_ROLE)).to.be.true;
expect(await token.hasRole(ownerWallet.address, MINTER_ROLE)).to.be.true;
expect(await token.interchainTokenId()).to.equal(tokenId);
expect(await token.interchainTokenService()).to.equal(service);
expect(await token.interchainTokenService()).to.equal(service.address);

await expectRevert(
(gasOptions) =>
Expand Down