Since low level calls do not check for contract existence, and return true if a contract does not exist, it's possible for Axelar proxy contracts that delegatecall to a setup function to fail silently if their implementationAddress does not exist.
// solhint-disable-next-line avoid-low-level-calls
(bool success, ) = implementationAddress.delegatecall(
//0x9ded06df is the setup selector.
abi.encodeWithSelector(0x9ded06df, params)
);
if (!success) revert SetupFailed();
Suggestion: Check address.code.length for implementationAddress. For example:
if (implementationAddress.code.length == 0) revert ImplementationDoesNotExist();
// solhint-disable-next-line avoid-low-level-calls
(bool success, ) = implementationAddress.delegatecall(
//0x9ded06df is the setup selector.
abi.encodeWithSelector(0x9ded06df, params)
);
if (!success) revert SetupFailed();
Gateway may be incompatible with rebasing tokens
In addition to fee-on-transfer tokens (see this finding from the previous C4 contest), the Axelar gateway is not fully compatible with ERC20 tokens that increase an account's underlying balance, like Lido stETH and Aave aTokens. Due to rebases, the gateway contract may accrue a balance greater than the token amount minted on a destination chain, and these excess amounts could remain locked in the gateway. There is not really a reasonable technical solution here: instead it's probably best to document this incompatibility for end users of the protocol if token creation becomes permissionless, or avoid these tokens if it is permissioned.
Fixed chain ID in domain separator
The ERC20Permit contract generates and permanently stores an EIP-712 domain separator using block.chainid at construction time:
Since chainid is set permanently, there is a risk of replay attacks in the event of a hard fork: since contracts on both forks would use the same stored chainid, signatures on one chain would be replayable on the fork. This is a pretty low probablity scenario, but as a cross-chain protocol supporting many EVM chains, Axelar is probably at higher risk of exposure to a hard fork than a typical single chain application.
Suggestion: cache chainId and DOMAIN_SEPARATOR at construction time. Use the cached DOMAIN_SEPARATOR unless chainID has changed. If so, regenerate DOMAIN_SEPARATOR.
QA
Extract a SafeTransfer library
A similar ERC20 _safeTransfer function is defined twice, in both DepositBase and AxelarGasService. Consider extracting this function to a shared library. (Note that the version in AxelarGasService includes a zero amount check that should be moved outside the function if you do implement this refactor).
Low
Proxy
setup
may fail silentlySince low level calls do not check for contract existence, and return
true
if a contract does not exist, it's possible for Axelar proxy contracts thatdelegatecall
to asetup
function to fail silently if theirimplementationAddress
does not exist.util/Proxy#init
:xc20
Proxy#constructor
:Suggestion: Check
address.code.length
forimplementationAddress
. For example:Gateway may be incompatible with rebasing tokens
In addition to fee-on-transfer tokens (see this finding from the previous C4 contest), the Axelar gateway is not fully compatible with ERC20 tokens that increase an account's underlying balance, like Lido stETH and Aave aTokens. Due to rebases, the gateway contract may accrue a balance greater than the token amount minted on a destination chain, and these excess amounts could remain locked in the gateway. There is not really a reasonable technical solution here: instead it's probably best to document this incompatibility for end users of the protocol if token creation becomes permissionless, or avoid these tokens if it is permissioned.
Fixed chain ID in domain separator
The
ERC20Permit
contract generates and permanently stores an EIP-712 domain separator usingblock.chainid
at construction time:ERC20Permit#constructor
ERC20Permit#constructor
Since
chainid
is set permanently, there is a risk of replay attacks in the event of a hard fork: since contracts on both forks would use the same storedchainid
, signatures on one chain would be replayable on the fork. This is a pretty low probablity scenario, but as a cross-chain protocol supporting many EVM chains, Axelar is probably at higher risk of exposure to a hard fork than a typical single chain application.Suggestion: cache
chainId
andDOMAIN_SEPARATOR
at construction time. Use the cachedDOMAIN_SEPARATOR
unlesschainID
has changed. If so, regenerateDOMAIN_SEPARATOR
.QA
Extract a
SafeTransfer
libraryA similar ERC20
_safeTransfer
function is defined twice, in bothDepositBase
andAxelarGasService
. Consider extracting this function to a shared library. (Note that the version inAxelarGasService
includes a zero amount check that should be moved outside the function if you do implement this refactor).DepositBase#_safeTransfer
:AxelarGasService#_safeTransfer
Note that a similar function exists in the XC20 contract as well, although it may make sense to keep it isolated:
XC20Wrapper.sol#_safeTransfer