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.

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: addressed ackee's report #312

Merged
merged 16 commits into from
Jan 6, 2025
Merged
3 changes: 2 additions & 1 deletion contracts/InterchainTokenFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,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.
* Cannot deploy tokens with empty supply and no minter.
* @param salt The unique salt for deploying the token.
* @param name The name of the token.
* @param symbol The symbol of the token.
Expand Down Expand Up @@ -150,7 +151,7 @@ contract InterchainTokenFactory is IInterchainTokenFactory, ITokenManagerType, M

minterBytes = minter.toBytes();
} else {
revert EmptyInterchainToken();
revert ZeroSupplyToken();
}

tokenId = _deployInterchainToken(deploySalt, currentChain, name, symbol, decimals, minterBytes, gasValue);
Expand Down
2 changes: 1 addition & 1 deletion contracts/InterchainTokenService.sol
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ contract InterchainTokenService is
}

/**
* @notice Executes operations based on the payload and selector.
* @notice Executes the cross-chain ITS message.
* @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.
Expand Down
10 changes: 0 additions & 10 deletions contracts/interchain-token/ERC20Permit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ 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)')
Expand Down Expand Up @@ -72,15 +71,6 @@ 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();
Expand Down
7 changes: 6 additions & 1 deletion contracts/interchain-token/InterchainToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

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';

Expand All @@ -13,7 +16,9 @@ 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, ERC20Permit, Minter, IInterchainToken {
contract InterchainToken is InterchainTokenStandard, ERC20, ERC20Permit, Minter, IInterchainToken {
using AddressBytes for bytes;

string public name;
string public symbol;
uint8 public decimals;
Expand Down
2 changes: 0 additions & 2 deletions contracts/interfaces/IBaseTokenManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ 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);
Expand Down
2 changes: 1 addition & 1 deletion contracts/interfaces/IInterchainTokenFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ interface IInterchainTokenFactory is IUpgradable, IMulticall {
error NotSupported();
error RemoteDeploymentNotApproved();
error InvalidTokenId(bytes32 tokenId, bytes32 expectedTokenId);
error EmptyInterchainToken();
error ZeroSupplyToken();

/// @notice Emitted when a minter approves a deployer for a remote interchain token deployment that uses a custom destinationMinter address.
event DeployRemoteInterchainTokenApproval(
Expand Down
8 changes: 6 additions & 2 deletions contracts/interfaces/ITokenManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,16 @@ import { IFlowLimit } from './IFlowLimit.sol';
interface ITokenManager is IBaseTokenManager, IOperator, IFlowLimit, IImplementation {
error TokenLinkerZeroAddress();
error NotService(address caller);
error ZeroTokenAddress();
error TakeTokenFailed();
error GiveTokenFailed();
error NotToken(address caller);
error ZeroAddress();
error AlreadyFlowLimiter(address flowLimiter);
error NotFlowLimiter(address flowLimiter);
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);
Expand Down
7 changes: 2 additions & 5 deletions contracts/token-manager/TokenManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ 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;
Expand Down Expand Up @@ -58,8 +57,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. It is included here so that the interace shows this function as existing, for better UX.
* @dev This function is not supported when directly called on the implementation. It
* must be called by the proxy.
* @return tokenAddress_ The address of the token.
*/
function tokenAddress() external view virtual returns (address) {
Expand All @@ -69,7 +68,6 @@ 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) {
Expand All @@ -91,7 +89,6 @@ 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();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test/InterchainToken.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ describe('InterchainToken', () => {
const contractBytecodeHash = keccak256(contractBytecode);

const expected = {
london: '0x482146829055f052063003e9cf0ffaf798a12fb58088c2667566a135b9568355',
london: '0xa01cf28b0b6ce6dc3b466e995585d69486400d671fce0ea8d06beba583e6f3bb',
Foivos marked this conversation as resolved.
Show resolved Hide resolved
}[getEVMVersion()];

expect(contractBytecodeHash).to.be.equal(expected);
Expand Down
4 changes: 2 additions & 2 deletions test/InterchainTokenFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,14 @@ describe('InterchainTokenFactory', () => {
await checkRoles(tokenManager, minter);
});

it('Should register a token if the mint amount is zero and minter is the zero address', async () => {
it('Should revert when trying to register a token if the mint amount is zero and minter is the zero address', async () => {
const salt = keccak256('0x123456');
tokenId = await tokenFactory.interchainTokenId(wallet.address, salt);

await expectRevert(
(gasOptions) => tokenFactory.deployInterchainToken(salt, name, symbol, decimals, 0, AddressZero, { gasOptions }),
tokenFactory,
'EmptyInterchainToken',
'ZeroSupplyToken',
[],
);
});
Expand Down
4 changes: 2 additions & 2 deletions test/InterchainTokenService.js
Original file line number Diff line number Diff line change
Expand Up @@ -909,13 +909,13 @@ describe('Interchain Token Service', () => {
);
});

it('Should revert on deploying a token manager with an empty token', async () => {
it('Should revert on deploying a token manager if token handler post deploy fails', async () => {
const params = defaultAbiCoder.encode(['bytes', 'address'], [wallet.address, AddressZero]);

await expectRevert(
(gasOptions) => service.deployTokenManager(salt, '', LOCK_UNLOCK, params, 0, gasOptions),
service,
'TokenManagerDeploymentFailed',
'PostDeployFailed',
);
});

Expand Down
2 changes: 1 addition & 1 deletion test/TokenManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe('Token Manager', () => {
const proxyBytecodeHash = keccak256(proxyBytecode);

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

expect(proxyBytecodeHash).to.be.equal(expected);
Expand Down
Loading