From 6c9056598b9bc78121c7e70acce51c4b8cf308ce Mon Sep 17 00:00:00 2001 From: Foivos Date: Tue, 10 Dec 2024 16:02:00 +0200 Subject: [PATCH] Addressed ackee's report. --- contracts/InterchainTokenFactory.sol | 2 + contracts/InterchainTokenService.sol | 40 +++++++++---------- contracts/TokenHandler.sol | 3 +- contracts/interchain-token/ERC20Permit.sol | 10 +++++ .../interchain-token/InterchainToken.sol | 7 +--- contracts/interfaces/IBaseTokenManager.sol | 2 + .../interfaces/IInterchainTokenFactory.sol | 3 +- .../interfaces/IInterchainTokenService.sol | 1 - contracts/interfaces/ITokenManager.sol | 8 +--- .../interfaces/ITokenManagerDeployer.sol | 1 - contracts/token-manager/TokenManager.sol | 7 +++- .../types/InterchainTokenServiceTypes.sol | 4 +- 12 files changed, 47 insertions(+), 41 deletions(-) diff --git a/contracts/InterchainTokenFactory.sol b/contracts/InterchainTokenFactory.sol index f3be98d3..31b4422b 100644 --- a/contracts/InterchainTokenFactory.sol +++ b/contracts/InterchainTokenFactory.sol @@ -149,6 +149,8 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M if (minter == address(interchainTokenService)) revert InvalidMinter(minter); minterBytes = minter.toBytes(); + } else { + revert EmptyInterchainToken(); } tokenId = _deployInterchainToken(deploySalt, currentChain, name, symbol, decimals, minterBytes, gasValue); diff --git a/contracts/InterchainTokenService.sol b/contracts/InterchainTokenService.sol index 7c83b429..fff6d471 100644 --- a/contracts/InterchainTokenService.sol +++ b/contracts/InterchainTokenService.sol @@ -395,6 +395,26 @@ contract InterchainTokenService is return _contractCallValue(payload); } + /** + * @notice Executes operations based on the payload and selector. + * @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. + * @param payload The encoded data payload for the transaction. + */ + function execute( + bytes32 commandId, + string calldata sourceChain, + string calldata sourceAddress, + bytes calldata payload + ) external onlyRemoteService(sourceChain, sourceAddress) whenNotPaused { + bytes32 payloadHash = keccak256(payload); + + if (!gateway.validateContractCall(commandId, sourceChain, sourceAddress, payloadHash)) revert NotApprovedByGateway(); + + _execute(commandId, sourceChain, sourceAddress, payload, payloadHash); + } + /** * @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. @@ -646,26 +666,6 @@ contract InterchainTokenService is } } - /** - * @notice Executes operations based on the payload and selector. - * @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. - * @param payload The encoded data payload for the transaction. - */ - function execute( - bytes32 commandId, - string calldata sourceChain, - string calldata sourceAddress, - bytes calldata payload - ) external onlyRemoteService(sourceChain, sourceAddress) whenNotPaused { - bytes32 payloadHash = keccak256(payload); - - if (!gateway.validateContractCall(commandId, sourceChain, sourceAddress, payloadHash)) revert NotApprovedByGateway(); - - _execute(commandId, sourceChain, sourceAddress, payload, payloadHash); - } - /** * @notice Processes the payload data for a send token call. * @param commandId The unique message id. diff --git a/contracts/TokenHandler.sol b/contracts/TokenHandler.sol index be794e9a..69932021 100644 --- a/contracts/TokenHandler.sol +++ b/contracts/TokenHandler.sol @@ -16,12 +16,11 @@ import { IERC20BurnableFrom } from './interfaces/IERC20BurnableFrom.sol'; /** * @title TokenHandler - * @notice This interface is responsible for handling tokens before initiating an interchain token transfer, or after receiving one. + * @notice This contract is responsible for handling tokens before initiating an interchain token transfer, or after receiving one. */ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard, Create3AddressFixed { using SafeTokenTransferFrom for IERC20; using SafeTokenCall for IERC20; - using SafeTokenTransfer for IERC20; /** * @notice This function gives token to a specified address from the token manager. diff --git a/contracts/interchain-token/ERC20Permit.sol b/contracts/interchain-token/ERC20Permit.sol index b692d723..00826284 100644 --- a/contracts/interchain-token/ERC20Permit.sol +++ b/contracts/interchain-token/ERC20Permit.sol @@ -23,6 +23,7 @@ abstract contract ERC20Permit is IERC20, IERC20Permit, ERC20 { */ bytes32 public nameHash; + // Prefix for the EIP712 structured data. string private constant EIP191_PREFIX_FOR_EIP712_STRUCTURED_DATA = '\x19\x01'; // keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)') @@ -71,6 +72,15 @@ abstract contract ERC20Permit is IERC20, IERC20Permit, ERC20 { function permit(address issuer, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external { if (block.timestamp > deadline) revert PermitExpired(); + // EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature + // unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines + // the valid range for s in (281): 0 < s < secp256k1n ÷ 2 + 1, and for v in (282): v ∈ {27, 28}. Most + // signatures from current libraries generate a unique signature with an s-value in the lower half order. + // + // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value + // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or + // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept + // these malleable signatures as well. if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) revert InvalidS(); if (v != 27 && v != 28) revert InvalidV(); diff --git a/contracts/interchain-token/InterchainToken.sol b/contracts/interchain-token/InterchainToken.sol index 1dea362a..03265886 100644 --- a/contracts/interchain-token/InterchainToken.sol +++ b/contracts/interchain-token/InterchainToken.sol @@ -2,12 +2,9 @@ pragma solidity ^0.8.0; -import { AddressBytes } from '@axelar-network/axelar-gmp-sdk-solidity/contracts/libs/AddressBytes.sol'; - import { IInterchainToken } from '../interfaces/IInterchainToken.sol'; import { InterchainTokenStandard } from './InterchainTokenStandard.sol'; -import { ERC20 } from './ERC20.sol'; import { ERC20Permit } from './ERC20Permit.sol'; import { Minter } from '../utils/Minter.sol'; @@ -16,9 +13,7 @@ import { Minter } from '../utils/Minter.sol'; * @notice This contract implements an interchain token which extends InterchainToken functionality. * @dev This contract also inherits Minter and Implementation logic. */ -contract InterchainToken is InterchainTokenStandard, ERC20, ERC20Permit, Minter, IInterchainToken { - using AddressBytes for bytes; - +contract InterchainToken is InterchainTokenStandard, ERC20Permit, Minter, IInterchainToken { string public name; string public symbol; uint8 public decimals; diff --git a/contracts/interfaces/IBaseTokenManager.sol b/contracts/interfaces/IBaseTokenManager.sol index 2ceff77d..b3dba446 100644 --- a/contracts/interfaces/IBaseTokenManager.sol +++ b/contracts/interfaces/IBaseTokenManager.sol @@ -9,12 +9,14 @@ pragma solidity ^0.8.0; interface IBaseTokenManager { /** * @notice A function that returns the token id. + * @dev This is stored in the proxy and should not be called in the implementation, but it is included here so that the interface properly tells us what functions exist. */ function interchainTokenId() external view returns (bytes32); /** * @notice A function that should return the address of the token. * Must be overridden in the inheriting contract. + * @dev This is stored in the proxy and should not be called in the implementation, but it is included here so that the interface properly tells us what functions exist. * @return address address of the token. */ function tokenAddress() external view returns (address); diff --git a/contracts/interfaces/IInterchainTokenFactory.sol b/contracts/interfaces/IInterchainTokenFactory.sol index e425ecbf..0db2aa76 100644 --- a/contracts/interfaces/IInterchainTokenFactory.sol +++ b/contracts/interfaces/IInterchainTokenFactory.sol @@ -16,11 +16,10 @@ interface IInterchainTokenFactory is IUpgradable, IMulticall { error InvalidChainName(); error InvalidMinter(address minter); error NotMinter(address minter); - error NotOperator(address operator); - error NotServiceOwner(address sender); error NotSupported(); error RemoteDeploymentNotApproved(); error InvalidTokenId(bytes32 tokenId, bytes32 expectedTokenId); + error EmptyInterchainToken(); /// @notice Emitted when a minter approves a deployer for a remote interchain token deployment that uses a custom destinationMinter address. event DeployRemoteInterchainTokenApproval( diff --git a/contracts/interfaces/IInterchainTokenService.sol b/contracts/interfaces/IInterchainTokenService.sol index b3f5fc12..f6aef9cd 100644 --- a/contracts/interfaces/IInterchainTokenService.sol +++ b/contracts/interfaces/IInterchainTokenService.sol @@ -29,7 +29,6 @@ interface IInterchainTokenService is IAddressTracker, IUpgradable { - error InvalidTokenManagerImplementationType(address implementation); error InvalidChainName(); error NotRemoteService(); error TokenManagerDoesNotExist(bytes32 tokenId); diff --git a/contracts/interfaces/ITokenManager.sol b/contracts/interfaces/ITokenManager.sol index 50c582a9..23c778b2 100644 --- a/contracts/interfaces/ITokenManager.sol +++ b/contracts/interfaces/ITokenManager.sol @@ -15,16 +15,12 @@ import { IFlowLimit } from './IFlowLimit.sol'; interface ITokenManager is IBaseTokenManager, IOperator, IFlowLimit, IImplementation { error TokenLinkerZeroAddress(); error NotService(address caller); - error TakeTokenFailed(); - error GiveTokenFailed(); - error NotToken(address caller); - error ZeroAddress(); - error AlreadyFlowLimiter(address flowLimiter); - error NotFlowLimiter(address flowLimiter); + error ZeroTokenAddress(); error NotSupported(); /** * @notice Returns implementation type of this token manager. + * @dev This is stored in the proxy and should not be called in the implementation, but it is included here so that the interface properly tells us what functions exist. * @return uint256 The implementation type of this token manager. */ function implementationType() external view returns (uint256); diff --git a/contracts/interfaces/ITokenManagerDeployer.sol b/contracts/interfaces/ITokenManagerDeployer.sol index 8e9acdd6..5478d0c1 100644 --- a/contracts/interfaces/ITokenManagerDeployer.sol +++ b/contracts/interfaces/ITokenManagerDeployer.sol @@ -7,7 +7,6 @@ pragma solidity ^0.8.0; * @notice This interface is used to deploy new instances of the TokenManagerProxy contract. */ interface ITokenManagerDeployer { - error AddressZero(); error TokenManagerDeploymentFailed(); /** diff --git a/contracts/token-manager/TokenManager.sol b/contracts/token-manager/TokenManager.sol index e6d30174..dbcc61db 100644 --- a/contracts/token-manager/TokenManager.sol +++ b/contracts/token-manager/TokenManager.sol @@ -18,6 +18,7 @@ import { FlowLimit } from '../utils/FlowLimit.sol'; /** * @title TokenManager * @notice This contract is responsible for managing tokens, such as setting locking token balances, or setting flow limits, for interchain transfers. + * @dev Should only be used as an implementation for TokenManagerProxy. */ contract TokenManager is ITokenManager, Operator, FlowLimit, Implementation, Multicall { using AddressBytes for bytes; @@ -57,8 +58,8 @@ contract TokenManager is ITokenManager, Operator, FlowLimit, Implementation, Mul /** * @notice Reads the token address from the proxy. - * @dev This function is not supported when directly called on the implementation. It - * must be called by the proxy. + * @dev This function is not supported when directly called on the implementation. + * It must be called by the proxy. It is included here so that the interace shows this function as existing, for better UX. * @return tokenAddress_ The address of the token. */ function tokenAddress() external view virtual returns (address) { @@ -68,6 +69,7 @@ contract TokenManager is ITokenManager, Operator, FlowLimit, Implementation, Mul /** * @notice A function that returns the token id. * @dev This will only work when implementation is called by a proxy, which stores the tokenId as an immutable. + * It is included here so that the interace shows this function as existing, for better UX. * @return bytes32 The interchain token ID. */ function interchainTokenId() public pure returns (bytes32) { @@ -89,6 +91,7 @@ contract TokenManager is ITokenManager, Operator, FlowLimit, Implementation, Mul */ function getTokenAddressFromParams(bytes calldata params_) external pure returns (address tokenAddress_) { (, tokenAddress_) = abi.decode(params_, (bytes, address)); + if (tokenAddress_ == address(0)) revert ZeroTokenAddress(); } /** diff --git a/contracts/types/InterchainTokenServiceTypes.sol b/contracts/types/InterchainTokenServiceTypes.sol index 9e293028..89d886ef 100644 --- a/contracts/types/InterchainTokenServiceTypes.sol +++ b/contracts/types/InterchainTokenServiceTypes.sol @@ -5,7 +5,9 @@ pragma solidity ^0.8.0; enum MessageType { INTERCHAIN_TRANSFER, DEPLOY_INTERCHAIN_TOKEN, - DEPLOY_TOKEN_MANAGER + DEPLOY_TOKEN_MANAGER, + SEND_TO_HUB, + RECEIVE_FROM_HUB } struct InterchainTransfer {