Axelar Network v2 contest - TomJ's results

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L23 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L51 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L71 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/ReceiverImplementation.sol#L86 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L128 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/gas-service/AxelarGasService.sol#L144 https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/xc20/contracts/XC20Wrapper.sol#L63

Vulnerability details

Impact

It is strongly recommended to avoid using payable.transfer, since it can cause the transaction to fail when:

1. The calling contract does not have payable function
2. The calling contract have a payable function but spends more than 2300 gas
3. The calling contract have a payable function and spends less than 2300 gas but is called through proxy which uses over 2300 gas.

Also it might be mandatory for some multisig wallets to use higher than 2300 gas. Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

./ReceiverImplementation.sol:23:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:51:            if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:71:        if (address(this).balance > 0) refundAddress.transfer(address(this).balance);
./ReceiverImplementation.sol:86:        recipient.transfer(amount);
./AxelarGasService.sol:128:                if (amount > 0) receiver.transfer(amount);
./AxelarGasService.sol:144:            receiver.transfer(amount);
./XC20Wrapper.sol:63:        payable(msg.sender).transfer(address(this).balance);

Tools Used

Manual Analysis

Recommended Mitigation Steps

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

Table of Contents

• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Use Calldata instead of Memory for Read Only Function Parameters
• Constant Value of a Call to keccak256() should Use Immutable
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Unnecessary Default Value Initialization
• ++i Costs Less Gas than i++
• != 0 costs less gass than > 0

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

1. Cache epochForHash[newOperatorsHash] of _transferOperatorship() AxelarAuthWeighted.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L76 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/auth/AxelarAuthWeighted.sol#L81

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

1. Remove 'gatewayToken' variable of refundTokenDeposit() AxelarDepositService.sol https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/AxelarDepositService.sol#L115 https://github.com/code-423n4/2022-07-axelar/blob/main/contracts/deposit-service/AxelarDepositService.sol#L118 Mitigation: Delete line 115 and replace line 118 like shown below

if (refundTokens[i] == IAxelarGateway(gateway).tokenAddresses(tokenSymbol) && msg.sender != refundAddress) continue;

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./AxelarAuthWeighted.sol:16:    constructor(bytes[] memory recentOperators) {
./AxelarAuthWeighted.sol:55:    function _transferOperatorship(bytes memory params) internal {
./AxelarAuthWeighted.sol:88:        address[] memory operators,
./AxelarAuthWeighted.sol:89:        uint256[] memory weights,
./AxelarAuthWeighted.sol:91:        bytes[] memory signatures
./AxelarAuthWeighted.sol:115:    function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {
./ReceiverImplementation.sol:12:    constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {}
./DepositBase.sol:21:    constructor(address gateway_, string memory wrappedSymbol_) {
./AxelarGasService.sol:40:        string memory symbol,
./XC20Wrapper.sol:51:        string memory newName,
./XC20Wrapper.sol:52:        string memory newSymbol
./AxelarDepositService.sol:18:    constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {
./AxelarDepositService.sol:220:    function _depositAddress(bytes32 create2Salt, bytes memory delegateData) internal view returns (address) {
./AxelarGateway.sol:152:    function tokenDailyMintLimit(string memory symbol) public view override returns (uint256) {
./AxelarGateway.sol:156:    function tokenDailyMintAmount(string memory symbol) public view override returns (uint256) {
./AxelarGateway.sol:168:    function tokenAddresses(string memory symbol) public view override returns (address) {
./AxelarGateway.sol:172:    function tokenFrozen(string memory) external pure override returns (bool) {
./AxelarGateway.sol:277:            bytes32[] memory commandIds_,
./AxelarGateway.sol:278:            string[] memory commands_,
./AxelarGateway.sol:279:            bytes[] memory params_
./AxelarGateway.sol:399:            string memory sourceChain,
./AxelarGateway.sol:400:            string memory sourceAddress,
./AxelarGateway.sol:413:            string memory sourceChain,
./AxelarGateway.sol:414:            string memory sourceAddress,
./AxelarGateway.sol:417:            string memory symbol,
./AxelarGateway.sol:447:    function _unpackLegacyCommands(bytes memory executeData)
./AxelarGateway.sol:452:            bytes32[] memory commandIds,
./AxelarGateway.sol:453:            string[] memory commands,
./AxelarGateway.sol:454:            bytes[] memory params
./AxelarGateway.sol:460:    function _callERC20Token(address tokenAddress, bytes memory callData) internal returns (bool) {
./AxelarGateway.sol:466:        string memory symbol,
./AxelarGateway.sol:487:        string memory symbol,
./AxelarGateway.sol:539:    function _getTokenDailyMintLimitKey(string memory symbol) internal pure returns (bytes32) {
./AxelarGateway.sol:543:    function _getTokenDailyMintAmountKey(string memory symbol, uint256 day) internal pure returns (bytes32) {
./AxelarGateway.sol:547:    function _getTokenTypeKey(string memory symbol) internal pure returns (bytes32) {
./AxelarGateway.sol:551:    function _getTokenAddressKey(string memory symbol) internal pure returns (bytes32) {
./AxelarGateway.sol:561:        string memory sourceChain,
./AxelarGateway.sol:562:        string memory sourceAddress,
./AxelarGateway.sol:571:        string memory sourceChain,
./AxelarGateway.sol:572:        string memory sourceAddress,
./AxelarGateway.sol:575:        string memory symbol,
./AxelarGateway.sol:597:    function _getTokenType(string memory symbol) internal view returns (TokenType) {
./AxelarGateway.sol:605:    function _setTokenDailyMintLimit(string memory symbol, uint256 limit) internal {
./AxelarGateway.sol:611:    function _setTokenDailyMintAmount(string memory symbol, uint256 amount) internal {
./AxelarGateway.sol:618:    function _setTokenType(string memory symbol, TokenType tokenType) internal {
./AxelarGateway.sol:622:    function _setTokenAddress(string memory symbol, address tokenAddress) internal {
./AxelarGateway.sol:632:        string memory sourceChain,
./AxelarGateway.sol:633:        string memory sourceAddress,
./AxelarGateway.sol:642:        string memory sourceChain,
./AxelarGateway.sol:643:        string memory sourceAddress,
./AxelarGateway.sol:646:        string memory symbol,
./DepositReceiver.sol:8:    constructor(bytes memory delegateData) {

Mitigation

Change memory to calldata

 

Constant Value of a Call to keccak256() should Use Immutable

Issue

When using constant it is expected that the value should be converted into a constant value at compile time. However when using a call to keccak256(), the expression is re-calculated each time the constant is referenced. Resulting in costing about 100 gas more on each access to this constant. link for more details: https://github.com/ethereum/solidity/issues/9232

PoC

Total of 15 issues found.

./AxelarGateway.sol:23:    // bytes32 internal constant KEY_ALL_TOKENS_FROZEN = keccak256('all-tokens-frozen');
./AxelarGateway.sol:24:    // bytes32 internal constant PREFIX_TOKEN_FROZEN = keccak256('token-frozen');
./AxelarGateway.sol:30:    bytes32 internal constant PREFIX_COMMAND_EXECUTED = keccak256('command-executed');
./AxelarGateway.sol:31:    bytes32 internal constant PREFIX_TOKEN_ADDRESS = keccak256('token-address');
./AxelarGateway.sol:32:    bytes32 internal constant PREFIX_TOKEN_TYPE = keccak256('token-type');
./AxelarGateway.sol:33:    bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED = keccak256('contract-call-approved');
./AxelarGateway.sol:34:    bytes32 internal constant PREFIX_CONTRACT_CALL_APPROVED_WITH_MINT = keccak256('contract-call-approved-with-mint');
./AxelarGateway.sol:35:    bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_LIMIT = keccak256('token-daily-mint-limit');
./AxelarGateway.sol:36:    bytes32 internal constant PREFIX_TOKEN_DAILY_MINT_AMOUNT = keccak256('token-daily-mint-amount');
./AxelarGateway.sol:38:    bytes32 internal constant SELECTOR_BURN_TOKEN = keccak256('burnToken');
./AxelarGateway.sol:39:    bytes32 internal constant SELECTOR_DEPLOY_TOKEN = keccak256('deployToken');
./AxelarGateway.sol:40:    bytes32 internal constant SELECTOR_MINT_TOKEN = keccak256('mintToken');
./AxelarGateway.sol:41:    bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL = keccak256('approveContractCall');
./AxelarGateway.sol:42:    bytes32 internal constant SELECTOR_APPROVE_CONTRACT_CALL_WITH_MINT = keccak256('approveContractCallWithMint');
./AxelarGateway.sol:43:    bytes32 internal constant SELECTOR_TRANSFER_OPERATORSHIP = keccak256('transferOperatorship');

Mitigation

Change the variable from constant to immutable.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, it acctually cost more gas to use elements smaller than 32 bytes. Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 1 issue found.

./AxelarAuthWeighted.sol:14:    uint8 internal constant OLD_KEY_RETENTION = 16;

Mitigation

I suggest using uint256 instead of anything smaller.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 6 issues found.

./AxelarAuthWeighted.sol:68:        uint256 totalWeight = 0;
./AxelarAuthWeighted.sol:69:        for (uint256 i = 0; i < weightsLength; ++i) {
./AxelarAuthWeighted.sol:94:        uint256 operatorIndex = 0;
./AxelarAuthWeighted.sol:95:        uint256 weight = 0;
./AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
./AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

Mitigation

I suggest removing default value initialization. For example,

• uint256 totalWeight;

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

Total of 5 issues found.

./AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {
./AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
./AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
./AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
./AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

Mitigation

Change it to postfix increments/decrements. For example,

for (uint256 i; i < tokens.length; ++i) {