From cd3af0955bd2815a338b91b7735f2c81b6a356ad Mon Sep 17 00:00:00 2001 From: ahramy Date: Sun, 10 Nov 2024 22:42:07 -0800 Subject: [PATCH 1/2] feat(its): add validation checks for evm its message fields (#300) --- contracts/InterchainTokenService.sol | 10 +++ .../interfaces/IInterchainTokenService.sol | 4 + test/InterchainTokenService.js | 86 +++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/contracts/InterchainTokenService.sol b/contracts/InterchainTokenService.sol index efd337cf..a2e2c342 100644 --- a/contracts/InterchainTokenService.sol +++ b/contracts/InterchainTokenService.sol @@ -297,6 +297,8 @@ contract InterchainTokenService is bytes calldata params, uint256 gasValue ) external payable whenNotPaused returns (bytes32 tokenId) { + if (bytes(params).length == 0) revert EmptyParams(); + // Custom token managers can't be deployed with native interchain token type, which is reserved for interchain tokens if (tokenManagerType == TokenManagerType.NATIVE_INTERCHAIN_TOKEN) revert CannotDeploy(tokenManagerType); @@ -515,6 +517,7 @@ contract InterchainTokenService is uint256 gasValue ) external payable whenNotPaused { if (data.length == 0) revert EmptyData(); + amount = _takeToken(tokenId, msg.sender, amount, false); _transmitInterchainTransfer( @@ -907,6 +910,9 @@ contract InterchainTokenService is string calldata destinationChain, uint256 gasValue ) internal { + if (bytes(name).length == 0) revert EmptyTokenName(); + if (bytes(symbol).length == 0) revert EmptyTokenSymbol(); + // slither-disable-next-line unused-return deployedTokenManager(tokenId); @@ -968,6 +974,9 @@ contract InterchainTokenService is string memory symbol, uint8 decimals ) internal returns (address tokenAddress) { + if (bytes(name).length == 0) revert EmptyTokenName(); + if (bytes(symbol).length == 0) revert EmptyTokenSymbol(); + bytes32 salt = _getInterchainTokenSalt(tokenId); address minter; @@ -1029,6 +1038,7 @@ contract InterchainTokenService is bytes memory data, uint256 gasValue ) internal { + if (destinationAddress.length == 0) revert EmptyDestinationAddress(); if (amount == 0) revert ZeroAmount(); // slither-disable-next-line reentrancy-events diff --git a/contracts/interfaces/IInterchainTokenService.sol b/contracts/interfaces/IInterchainTokenService.sol index 54dadff1..d2c52cab 100644 --- a/contracts/interfaces/IInterchainTokenService.sol +++ b/contracts/interfaces/IInterchainTokenService.sol @@ -50,6 +50,10 @@ interface IInterchainTokenService is error CannotDeployRemotelyToSelf(); error InvalidPayload(); error GatewayCallFailed(bytes data); + error EmptyTokenName(); + error EmptyTokenSymbol(); + error EmptyParams(); + error EmptyDestinationAddress(); event InterchainTransfer( bytes32 indexed tokenId, diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index 1eafa9d8..63b124ee 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -675,6 +675,26 @@ describe('Interchain Token Service', () => { await service.setPauseStatus(false).then((tx) => tx.wait); }); + + it('Should revert when registering an interchain token with empty token name', async () => { + expect(await tokenManager.hasRole(wallet.address, OPERATOR_ROLE)).to.be.true; + + await expectRevert( + (gasOptions) => service.deployInterchainToken(salt, '', '', tokenSymbol, tokenDecimals, wallet.address, 0, gasOptions), + service, + 'EmptyTokenName', + ); + }); + + it('Should revert when registering an interchain token with empty token symbol', async () => { + expect(await tokenManager.hasRole(wallet.address, OPERATOR_ROLE)).to.be.true; + + await expectRevert( + (gasOptions) => service.deployInterchainToken(salt, '', tokenName, '', tokenDecimals, wallet.address, 0, gasOptions), + service, + 'EmptyTokenSymbol', + ); + }); }); describe('Deploy and Register remote Interchain Token', () => { @@ -728,6 +748,30 @@ describe('Interchain Token Service', () => { ); }); + it('Should revert on remote interchain token deployment with invalid token name', async () => { + await expectRevert( + (gasOptions) => + service.deployInterchainToken(salt, destinationChain, '', tokenSymbol, tokenDecimals, minter, gasValue, { + ...gasOptions, + value: gasValue, + }), + service, + 'EmptyTokenName', + ); + }); + + it('Should revert on remote interchain token deployment with invalid token symbol', async () => { + await expectRevert( + (gasOptions) => + service.deployInterchainToken(salt, destinationChain, tokenName, '', tokenDecimals, minter, gasValue, { + ...gasOptions, + value: gasValue, + }), + service, + 'EmptyTokenSymbol', + ); + }); + it('Should revert on remote interchain token deployment if paused', async () => { await service.setPauseStatus(true).then((tx) => tx.wait); @@ -820,6 +864,14 @@ describe('Interchain Token Service', () => { await expectRevert((gasOptions) => service.deployTokenManager(salt, '', 6, params, 0, gasOptions)); }); + it('Should revert on deploying a local token manager with invalid params', async () => { + await expectRevert( + (gasOptions) => service.deployTokenManager(salt, '', NATIVE_INTERCHAIN_TOKEN, '0x', 0, gasOptions), + service, + 'EmptyParams', + ); + }); + it('Should revert on deploying a local token manager with interchain token manager type', async () => { const params = defaultAbiCoder.encode(['bytes', 'address'], [wallet.address, token.address]); @@ -1271,6 +1323,18 @@ describe('Interchain Token Service', () => { ); }); + it('Should revert on initiate interchain token transfer with invalid destination address', async () => { + await expectRevert( + (gasOptions) => + service.interchainTransfer(tokenId, destinationChain, '0x', amount, '0x', gasValue, { + ...gasOptions, + value: gasValue, + }), + service, + 'EmptyDestinationAddress', + ); + }); + it('Should revert on initiate interchain token transfer when service is paused', async () => { await service.setPauseStatus(true).then((tx) => tx.wait); @@ -1299,6 +1363,18 @@ describe('Interchain Token Service', () => { await service.setPauseStatus(false).then((tx) => tx.wait); }); + it('Should revert on transmit send token when destination address is zero address', async () => { + await expectRevert( + (gasOptions) => + service.transmitInterchainTransfer(tokenId, wallet.address, destinationChain, '0x', amount, '0x', { + ...gasOptions, + value: gasValue, + }), + service, + 'TakeTokenFailed', + ); + }); + it('Should revert on transmit send token when not called by interchain token', async () => { const errorSignatureHash = id('NotToken(address,address)'); const selector = errorSignatureHash.substring(0, 10); @@ -1638,6 +1714,16 @@ describe('Interchain Token Service', () => { ); }); + it('Should revert on callContractWithInterchainToken function on the service with invalid destination address', async () => { + const [, , tokenId] = await deployFunctions.lockUnlock(service, 'Test Token', 'TT', 12, amount); + + await expectRevert( + (gasOptions) => service.callContractWithInterchainToken(tokenId, destinationChain, '0x', amount, data, 0, gasOptions), + service, + 'EmptyDestinationAddress', + ); + }); + for (const type of ['lockUnlock', 'lockUnlockFee']) { it(`Should be able to initiate an interchain token transfer via the interchainTransfer function on the service when the service is approved as well [${type}]`, async () => { const [token, tokenManager, tokenId] = await deployFunctions[type](service, `Test Token ${type}`, 'TT', 12, amount); From 3d0a37114892b17b58558e83ca0e51b4c8cdb221 Mon Sep 17 00:00:00 2001 From: ahramy Date: Sun, 10 Nov 2024 22:48:37 -0800 Subject: [PATCH 2/2] fix(its): remove unnecessary use of payable in functions (#298) --- contracts/InterchainTokenFactory.sol | 2 ++ contracts/InterchainTokenService.sol | 1 + contracts/TokenHandler.sol | 6 ++--- contracts/interfaces/ITokenHandler.sol | 5 ++-- test/InterchainTokenService.js | 35 +++++++++++++++++++++++++- 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/contracts/InterchainTokenFactory.sol b/contracts/InterchainTokenFactory.sol index ef936708..4905d96f 100644 --- a/contracts/InterchainTokenFactory.sol +++ b/contracts/InterchainTokenFactory.sol @@ -104,6 +104,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M /** * @notice Deploys a new interchain token with specified parameters. * @dev Creates a new token and optionally mints an initial amount to a specified minter. + * This function is `payable` because non-payable functions cannot be called in a multicall that calls other `payable` functions. * @param salt The unique salt for deploying the token. * @param name The name of the token. * @param symbol The symbol of the token. @@ -250,6 +251,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M /** * @notice Registers a canonical token as an interchain token and deploys its token manager. + * @dev This function is `payable` because non-payable functions cannot be called in a multicall that calls other `payable` functions. * @param tokenAddress The address of the canonical token. * @return tokenId The tokenId corresponding to the registered canonical token. */ diff --git a/contracts/InterchainTokenService.sol b/contracts/InterchainTokenService.sol index a2e2c342..df782123 100644 --- a/contracts/InterchainTokenService.sol +++ b/contracts/InterchainTokenService.sol @@ -384,6 +384,7 @@ contract InterchainTokenService is /** * @notice Express executes operations based on the payload and selector. + * @dev This function is `payable` because non-payable functions cannot be called in a multicall that calls other `payable` functions. * @param commandId The unique message id. * @param sourceChain The chain where the transaction originates from. * @param sourceAddress The address of the remote ITS where the transaction originates from. diff --git a/contracts/TokenHandler.sol b/contracts/TokenHandler.sol index 7bf1dae8..be794e9a 100644 --- a/contracts/TokenHandler.sol +++ b/contracts/TokenHandler.sol @@ -31,8 +31,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Crea * @return uint256 The amount of token actually given, which could be different for certain token type. * @return address the address of the token. */ - // slither-disable-next-line locked-ether - function giveToken(bytes32 tokenId, address to, uint256 amount) external payable returns (uint256, address) { + function giveToken(bytes32 tokenId, address to, uint256 amount) external returns (uint256, address) { address tokenManager = _create3Address(tokenId); (uint256 tokenManagerType, address tokenAddress) = ITokenManagerProxy(tokenManager).getImplementationTypeAndTokenAddress(); @@ -107,8 +106,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Crea * @return uint256 The amount of token actually transferred, which could be different for certain token type. * @return address The address of the token corresponding to the input tokenId. */ - // slither-disable-next-line locked-ether - function transferTokenFrom(bytes32 tokenId, address from, address to, uint256 amount) external payable returns (uint256, address) { + function transferTokenFrom(bytes32 tokenId, address from, address to, uint256 amount) external returns (uint256, address) { address tokenManager = _create3Address(tokenId); (uint256 tokenManagerType, address tokenAddress) = ITokenManagerProxy(tokenManager).getImplementationTypeAndTokenAddress(); diff --git a/contracts/interfaces/ITokenHandler.sol b/contracts/interfaces/ITokenHandler.sol index 2b7f3f3e..7650de21 100644 --- a/contracts/interfaces/ITokenHandler.sol +++ b/contracts/interfaces/ITokenHandler.sol @@ -18,7 +18,7 @@ interface ITokenHandler { * @return uint256 The amount of token actually given, which could be different for certain token type. * @return address the address of the token. */ - function giveToken(bytes32 tokenId, address to, uint256 amount) external payable returns (uint256, address); + function giveToken(bytes32 tokenId, address to, uint256 amount) external returns (uint256, address); /** * @notice This function takes token from a specified address to the token manager. @@ -40,8 +40,7 @@ interface ITokenHandler { * @return uint256 The amount of token actually transferred, which could be different for certain token type. * @return address The address of the token corresponding to the input tokenId. */ - // slither-disable-next-line locked-ether - function transferTokenFrom(bytes32 tokenId, address from, address to, uint256 amount) external payable returns (uint256, address); + function transferTokenFrom(bytes32 tokenId, address from, address to, uint256 amount) external returns (uint256, address); /** * @notice This function prepares a token manager after it is deployed diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index 63b124ee..a4c4445f 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -1773,7 +1773,9 @@ describe('Interchain Token Service', () => { await expect( reportGas( - service.callContractWithInterchainToken(tokenId, destinationChain, destAddress, amount, data, 0), + service.callContractWithInterchainToken(tokenId, destinationChain, destAddress, amount, data, 0, { + value: gasValue, + }), `Call service.callContractWithInterchainToken ${type}`, ), ) @@ -2172,6 +2174,37 @@ describe('Interchain Token Service', () => { }); } + for (const type of ['mintBurn', 'mintBurnFrom', 'lockUnlockFee', 'lockUnlock']) { + it(`Should be able to initiate an interchain token transfer via interchainTransfer [${type}] without native gas`, async () => { + [token, tokenManager, tokenId] = await deployFunctions[type](service, `Test Token ${type}`, 'TT', 12, amount * 3, true); + const sendAmount = type === 'lockUnlockFee' ? amount - 10 : amount; + const payload = defaultAbiCoder.encode( + ['uint256', 'bytes32', 'bytes', 'bytes', 'uint256', 'bytes'], + [MESSAGE_TYPE_INTERCHAIN_TRANSFER, tokenId, hexlify(wallet.address), destAddress, sendAmount, '0x'], + ); + const payloadHash = keccak256(payload); + + let transferToAddress = AddressZero; + + if (type === 'lockUnlock' || type === 'lockUnlockFee') { + transferToAddress = tokenManager.address; + } + + await expect( + reportGas( + token.connect(wallet).interchainTransfer(destinationChain, destAddress, amount, metadata), + `Call token.interchainTransfer ${type}`, + ), + ) + .and.to.emit(token, 'Transfer') + .withArgs(wallet.address, transferToAddress, amount) + .and.to.emit(gateway, 'ContractCall') + .withArgs(service.address, destinationChain, service.address, payloadHash, payload) + .to.emit(service, 'InterchainTransfer') + .withArgs(tokenId, wallet.address, destinationChain, destAddress, sendAmount, HashZero); + }); + } + it('Should be able to initiate an interchain token transfer using interchainTransferFrom with max possible allowance', async () => { const sendAmount = amount; const payload = defaultAbiCoder.encode(