From 89cf6390cc2a8b5ad6983b116318e367d6a0aeeb Mon Sep 17 00:00:00 2001 From: ahramy Date: Sat, 9 Nov 2024 00:45:09 -0800 Subject: [PATCH 1/2] feat(its): add validation checks for evm its message fields --- contracts/InterchainTokenService.sol | 15 +++ .../interfaces/IInterchainTokenService.sol | 5 + test/InterchainTokenService.js | 98 +++++++++++++++++++ 3 files changed, 118 insertions(+) diff --git a/contracts/InterchainTokenService.sol b/contracts/InterchainTokenService.sol index efd337cf..12873449 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); @@ -491,6 +493,8 @@ contract InterchainTokenService is bytes calldata metadata, uint256 gasValue ) external payable whenNotPaused { + if (destinationAddress.length == 0) revert EmptyDestinationAddress(); + amount = _takeToken(tokenId, msg.sender, amount, false); (IGatewayCaller.MetadataVersion metadataVersion, bytes memory data) = _decodeMetadata(metadata); @@ -514,7 +518,9 @@ contract InterchainTokenService is bytes memory data, uint256 gasValue ) external payable whenNotPaused { + if (destinationAddress.length == 0) revert EmptyDestinationAddress(); if (data.length == 0) revert EmptyData(); + amount = _takeToken(tokenId, msg.sender, amount, false); _transmitInterchainTransfer( @@ -551,6 +557,9 @@ contract InterchainTokenService is uint256 amount, bytes calldata metadata ) external payable whenNotPaused { + if (sourceAddress == address(0)) revert EmptySourceAddress(); + if (destinationAddress.length == 0) revert EmptyDestinationAddress(); + amount = _takeToken(tokenId, sourceAddress, amount, true); (IGatewayCaller.MetadataVersion metadataVersion, bytes memory data) = _decodeMetadata(metadata); @@ -907,6 +916,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 +980,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; diff --git a/contracts/interfaces/IInterchainTokenService.sol b/contracts/interfaces/IInterchainTokenService.sol index 54dadff1..61d89fb0 100644 --- a/contracts/interfaces/IInterchainTokenService.sol +++ b/contracts/interfaces/IInterchainTokenService.sol @@ -50,6 +50,11 @@ interface IInterchainTokenService is error CannotDeployRemotelyToSelf(); error InvalidPayload(); error GatewayCallFailed(bytes data); + error EmptyTokenName(); + error EmptyTokenSymbol(); + error EmptyParams(); + error EmptySourceAddress(); + error EmptyDestinationAddress(); event InterchainTransfer( bytes32 indexed tokenId, diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index 1eafa9d8..570b33c5 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,30 @@ describe('Interchain Token Service', () => { await service.setPauseStatus(false).then((tx) => tx.wait); }); + it('Should revert on transmit send token when source address is zero address', async () => { + await expectRevert( + (gasOptions) => + service.transmitInterchainTransfer(tokenId, AddressZero, destinationChain, destAddress, amount, '0x', { + ...gasOptions, + value: gasValue, + }), + service, + 'EmptySourceAddress', + ); + }); + + 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, + 'EmptyDestinationAddress', + ); + }); + 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 +1726,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 db1dc08e1370bdfe4e159720862dce02c97fd2c3 Mon Sep 17 00:00:00 2001 From: ahramy Date: Sun, 10 Nov 2024 18:28:04 -0800 Subject: [PATCH 2/2] updated --- contracts/InterchainTokenService.sol | 7 +------ contracts/interfaces/IInterchainTokenService.sol | 1 - test/InterchainTokenService.js | 14 +------------- 3 files changed, 2 insertions(+), 20 deletions(-) diff --git a/contracts/InterchainTokenService.sol b/contracts/InterchainTokenService.sol index 12873449..a2e2c342 100644 --- a/contracts/InterchainTokenService.sol +++ b/contracts/InterchainTokenService.sol @@ -493,8 +493,6 @@ contract InterchainTokenService is bytes calldata metadata, uint256 gasValue ) external payable whenNotPaused { - if (destinationAddress.length == 0) revert EmptyDestinationAddress(); - amount = _takeToken(tokenId, msg.sender, amount, false); (IGatewayCaller.MetadataVersion metadataVersion, bytes memory data) = _decodeMetadata(metadata); @@ -518,7 +516,6 @@ contract InterchainTokenService is bytes memory data, uint256 gasValue ) external payable whenNotPaused { - if (destinationAddress.length == 0) revert EmptyDestinationAddress(); if (data.length == 0) revert EmptyData(); amount = _takeToken(tokenId, msg.sender, amount, false); @@ -557,9 +554,6 @@ contract InterchainTokenService is uint256 amount, bytes calldata metadata ) external payable whenNotPaused { - if (sourceAddress == address(0)) revert EmptySourceAddress(); - if (destinationAddress.length == 0) revert EmptyDestinationAddress(); - amount = _takeToken(tokenId, sourceAddress, amount, true); (IGatewayCaller.MetadataVersion metadataVersion, bytes memory data) = _decodeMetadata(metadata); @@ -1044,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 61d89fb0..d2c52cab 100644 --- a/contracts/interfaces/IInterchainTokenService.sol +++ b/contracts/interfaces/IInterchainTokenService.sol @@ -53,7 +53,6 @@ interface IInterchainTokenService is error EmptyTokenName(); error EmptyTokenSymbol(); error EmptyParams(); - error EmptySourceAddress(); error EmptyDestinationAddress(); event InterchainTransfer( diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index 570b33c5..63b124ee 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -1363,18 +1363,6 @@ describe('Interchain Token Service', () => { await service.setPauseStatus(false).then((tx) => tx.wait); }); - it('Should revert on transmit send token when source address is zero address', async () => { - await expectRevert( - (gasOptions) => - service.transmitInterchainTransfer(tokenId, AddressZero, destinationChain, destAddress, amount, '0x', { - ...gasOptions, - value: gasValue, - }), - service, - 'EmptySourceAddress', - ); - }); - it('Should revert on transmit send token when destination address is zero address', async () => { await expectRevert( (gasOptions) => @@ -1383,7 +1371,7 @@ describe('Interchain Token Service', () => { value: gasValue, }), service, - 'EmptyDestinationAddress', + 'TakeTokenFailed', ); });