Axelar Network v2 contest - Dravee's results

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug
  • 1.2. Deprecated approve() function
  • 1.3. Missing address(0) checks
  • 1.4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 1.5. Use a 2-step ownership transfer pattern
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. Use bytes.concat()
  • 2.3. public functions not called by the contract should be declared external instead

1. Low Risk Issues

1.1. Low level calls with solidity version <= 0.8.14 can result in optimizer bug

The protocol is using low level calls with solidity version <= 0.8.14 which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Consider upgrading to solidity 0.8.15 here:

deposit-service/DepositBase.sol:3:pragma solidity 0.8.9;
deposit-service/DepositBase.sol:71:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
gas-service/AxelarGasService.sol:3:pragma solidity 0.8.9;
gas-service/AxelarGasService.sol:158:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
gas-service/AxelarGasService.sol:172:        (bool success, bytes memory returnData) = tokenAddress.call(
xc20/contracts/XC20Wrapper.sol:3:pragma solidity 0.8.9;
xc20/contracts/XC20Wrapper.sol:95:        (bool success, bytes memory returnData) = tokenAddress.call(abi.encodeWithSelector(IERC20.transfer.selector, receiver, amount));
xc20/contracts/XC20Wrapper.sol:106:        (bool success, bytes memory returnData) = tokenAddress.call(
AxelarGateway.sol:3:pragma solidity 0.8.9;
AxelarGateway.sol:320:            (bool success, ) = address(this).call(abi.encodeWithSelector(commandSelector, params[i], commandId));
AxelarGateway.sol:461:        (bool success, bytes memory returnData) = tokenAddress.call(callData);

1.2. Deprecated approve() function

Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

deposit-service/AxelarDepositService.sol:30:        IERC20(wrappedTokenAddress).approve(gateway, amount);
deposit-service/ReceiverImplementation.sol:38:        IERC20(tokenAddress).approve(gateway, amount);
deposit-service/ReceiverImplementation.sol:64:        IERC20(wrappedTokenAddress).approve(gateway, amount);

1.3. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• receiverImplementation

File: AxelarDepositService.sol
16:     address public immutable receiverImplementation;
17: 
18:     constructor(address gateway, string memory wrappedSymbol) DepositBase(gateway, wrappedSymbol) {
19:         receiverImplementation = address(new ReceiverImplementation(gateway, wrappedSymbol));
20:     }

• gatewayAddress

File: XC20Wrapper.sol
24:     address public immutable gatewayAddress;
25: 
26:     constructor(address gatewayAddress_) {
27:         gatewayAddress = gatewayAddress_;

This is already done here:

• gateway

File: DepositBase.sol
13:     address public immutable gateway;
...
21:     constructor(address gateway_, string memory wrappedSymbol_) {
22:         if (gateway_ == address(0)) revert InvalidAddress();

and here:

• AUTH_MODULE and TOKEN_DEPLOYER_IMPLEMENTATION

File: AxelarGateway.sol
44: 
45:     address internal immutable AUTH_MODULE;
46:     address internal immutable TOKEN_DEPLOYER_IMPLEMENTATION;
47: 
48:     constructor(address authModule, address tokenDeployerImplementation) {
49:         if (authModule.code.length == 0) revert InvalidAuthModule();
50:         if (tokenDeployerImplementation.code.length == 0) revert InvalidTokenDeployer();
51: 
52:         AUTH_MODULE = authModule;
53:         TOKEN_DEPLOYER_IMPLEMENTATION = tokenDeployerImplementation;
54:     }
55: 

1.4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here Original issue: Hash collisions when using abi.encodePacked() with multiple variable length arguments

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

AxelarGateway.sol:298:            bytes32 commandHash = keccak256(abi.encodePacked(commands[i]));

1.5. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

auth/AxelarAuthWeighted.sol:7:import { Ownable } from '../Ownable.sol';
auth/AxelarAuthWeighted.sol:9:contract AxelarAuthWeighted is Ownable, IAxelarAuthWeighted {
auth/AxelarAuthWeighted.sol:47:    function transferOperatorship(bytes calldata params) external onlyOwner {
gas-service/AxelarGasService.sol:120:    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
gas-service/AxelarGasService.sol:140:    ) external onlyOwner {
interfaces/IAxelarAuth.sol:5:import { IOwnable } from './IOwnable.sol';
interfaces/IAxelarAuth.sol:7:interface IAxelarAuth is IOwnable {
xc20/contracts/XC20Wrapper.sol:36:        _transferOwnership(owner_);
xc20/contracts/XC20Wrapper.sol:44:    function setXc20Codehash(bytes32 newCodehash) external onlyOwner {
xc20/contracts/XC20Wrapper.sol:53:    ) external payable onlyOwner {
xc20/contracts/XC20Wrapper.sol:66:    function removeWrapping(string calldata symbol) external onlyOwner {

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

The emit keyword is at line L224, consider moving it after L234:

File: AxelarGateway.sol
217:     function upgrade(
218:         address newImplementation,
219:         bytes32 newImplementationCodeHash,
220:         bytes calldata setupParams
221:     ) external override onlyAdmin {
222:         if (newImplementationCodeHash != newImplementation.codehash) revert InvalidCodeHash();
223: 
224:         emit Upgraded(newImplementation);
225: 
226:         // AUDIT: If `newImplementation.setup` performs `selfdestruct`, it will result in the loss of _this_ implementation (thereby losing the gateway)
227:         //        if `upgrade` is entered within the context of _this_ implementation itself.
228:         if (setupParams.length != 0) {
229:             (bool success, ) = newImplementation.delegatecall(abi.encodeWithSelector(IAxelarGateway.setup.selector, setupParams));
230: 
231:             if (!success) revert SetupFailed();
232:         }
233: 
234:         _setImplementation(newImplementation);
235:     }

2.2. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)), this is relevant. Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>)), but it isn't used in the solution, so the upgrade isn't relevant (0.8.9 is ok)

deposit-service/AxelarDepositService.sol:3:pragma solidity 0.8.9;
deposit-service/AxelarDepositService.sol:228:                            abi.encodePacked(
deposit-service/AxelarDepositService.sol:233:                                keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))
AxelarGateway.sol:3:pragma solidity 0.8.9;
AxelarGateway.sol:540:        return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_LIMIT, symbol));
AxelarGateway.sol:544:        return keccak256(abi.encodePacked(PREFIX_TOKEN_DAILY_MINT_AMOUNT, symbol, day));
AxelarGateway.sol:548:        return keccak256(abi.encodePacked(PREFIX_TOKEN_TYPE, symbol));
AxelarGateway.sol:552:        return keccak256(abi.encodePacked(PREFIX_TOKEN_ADDRESS, symbol));
AxelarGateway.sol:556:        return keccak256(abi.encodePacked(PREFIX_COMMAND_EXECUTED, commandId));

2.3. public functions not called by the contract should be declared external instead

deposit-service/AxelarDepositService.sol:241:    function contractId() public pure returns (bytes32) {
deposit-service/DepositBase.sol:41:    function wrappedToken() public view returns (address) {
deposit-service/DepositBase.sol:46:    function wrappedSymbol() public view returns (string memory symbol) {
xc20/contracts/XC20Wrapper.sol:30:    function gateway() public view override returns (IAxelarGateway) {
xc20/contracts/XC20Wrapper.sol:40:    function contractId() public pure returns (bytes32) {
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:164:    function implementation() public view override returns (address) {
AxelarGateway.sol:176:    function isCommandExecuted(bytes32 commandId) public view override returns (bool) {

Overview

Table of Contents:

• 1. Use EIP-1167 minimal proxies for 10x cheaper contract instantiation
• 2. Multiple accesses of a mapping/array should use a local variable cache
• 3. Caching storage values in memory
• 4. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 5. <array>.length should not be looked up in every loop of a for-loop
• 6. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 7. Increments/decrements can be unchecked in for-loops
• 8. Upgrade pragma
• 9. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked payable

1. Use EIP-1167 minimal proxies for 10x cheaper contract instantiation

When new contracts have to be instantiated frequently, it's much cheaper for it to be done via minimal proxies. The only downside is that they rely on delegatecall() calls for every function, which adds an overhead of ~800 gas, but this is multiple orders of magnitude less than the amount saved during deployment.

Due to the following note, this optimization seem relevant for DepositReceiver:

File: AxelarDepositService.sol
92:         // NOTE: `DepositReceiver` is destroyed in the same runtime context that it is deployed.

Affected code:

deposit-service/AxelarDepositService.sol:93:        new DepositReceiver{ salt: salt }(
deposit-service/AxelarDepositService.sol:123:            new DepositReceiver{ salt: salt }(
deposit-service/AxelarDepositService.sol:145:        new DepositReceiver{ salt: salt }(
deposit-service/AxelarDepositService.sol:171:            new DepositReceiver{ salt: salt }(
deposit-service/AxelarDepositService.sol:191:        new DepositReceiver{ salt: salt }(
deposit-service/AxelarDepositService.sol:212:            new DepositReceiver{ salt: salt }(

2. Multiple accesses of a mapping/array should use a local variable cache

Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.

Affected code:

• refundTokens[i] (calldata)

deposit-service/AxelarDepositService.sol:118:            if (refundTokens[i] == gatewayToken && msg.sender != refundAddress) continue;
deposit-service/AxelarDepositService.sol:121:            refundToken = refundTokens[i];
deposit-service/AxelarDepositService.sol:208:            if (refundTokens[i] == wrappedTokenAddress && msg.sender != refundAddress) continue;
deposit-service/AxelarDepositService.sol:210:            refundToken = refundTokens[i];

• wrapped[axelarToken] and unwrapped[xc20Token] (storage)

xc20/contracts/XC20Wrapper.sol:57:        if (wrapped[axelarToken] != address(0)) revert('AlreadyWrappingAxelarToken()');
xc20/contracts/XC20Wrapper.sol:58:        if (unwrapped[xc20Token] != address(0)) revert('AlreadyWrappingXC20Token()');
xc20/contracts/XC20Wrapper.sol:59:        wrapped[axelarToken] = xc20Token;
xc20/contracts/XC20Wrapper.sol:60:        unwrapped[xc20Token] = axelarToken;
xc20/contracts/XC20Wrapper.sol:69:        address xc20Token = wrapped[axelarToken];
xc20/contracts/XC20Wrapper.sol:71:        wrapped[axelarToken] = address(0);
xc20/contracts/XC20Wrapper.sol:72:        unwrapped[xc20Token] = address(0);
xc20/contracts/XC20Wrapper.sol:77:        address wrappedToken = wrapped[axelarToken];

3. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs (here, notably on the gateway state variable).

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• contracts/deposit-service/AxelarDepositService.sol:

  30:         IERC20(wrappedTokenAddress).approve(gateway, amount); //@audit gas SLOAD (gateway)
  31:         IAxelarGateway(gateway).sendToken(destinationChain, destinationAddress, wrappedSymbol(), amount); //@audit gas SLOAD (gateway)

• contracts/deposit-service/ReceiverImplementation.sol (1):

  25:         address tokenAddress = IAxelarGateway(gateway).tokenAddresses(symbol); //@audit gas SLOAD (gateway)
  38:         IERC20(tokenAddress).approve(gateway, amount); //@audit gas SLOAD (gateway)
  39:         IAxelarGateway(gateway).sendToken(destinationChain, destinationAddress, symbol, amount); //@audit gas SLOAD (gateway)

• contracts/deposit-service/ReceiverImplementation.sol (2):

  64:         IERC20(wrappedTokenAddress).approve(gateway, amount); //@audit gas SLOAD (gateway)
  65:         IAxelarGateway(gateway).sendToken(destinationChain, destinationAddress, wrappedSymbol(), amount); //@audit gas SLOAD (gateway)

4. Duplicated conditions should be refactored to a modifier or function to save deployment costs

deposit-service/ReceiverImplementation.sol:35:        if (amount == 0) revert NothingDeposited();
deposit-service/ReceiverImplementation.sol:60:        if (amount == 0) revert NothingDeposited();
deposit-service/ReceiverImplementation.sol:82:        if (amount == 0) revert NothingDeposited();

gas-service/AxelarGasService.sol:69:        if (msg.value == 0) revert NothingReceived();
gas-service/AxelarGasService.sol:84:        if (msg.value == 0) revert NothingReceived();
gas-service/AxelarGasService.sol:115:        if (msg.value == 0) revert NothingReceived();

gas-service/AxelarGasService.sol:121:        if (receiver == address(0)) revert InvalidAddress();
gas-service/AxelarGasService.sol:141:        if (receiver == address(0)) revert InvalidAddress();

gas-service/AxelarGasService.sol:155:        if (amount == 0) revert NothingReceived();
gas-service/AxelarGasService.sol:169:        if (amount == 0) revert NothingReceived();

gas-service/AxelarGasService.sol:161:        if (!transferred || tokenAddress.code.length == 0) revert TransferFailed();
gas-service/AxelarGasService.sol:177:        if (!transferred || tokenAddress.code.length == 0) revert TransferFailed();

xc20/contracts/XC20Wrapper.sol:68:        if (axelarToken == address(0)) revert('NotAxelarToken()');
xc20/contracts/XC20Wrapper.sol:78:        if (wrappedToken == address(0)) revert('NotAxelarToken()');

xc20/contracts/XC20Wrapper.sol:98:        if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');
xc20/contracts/XC20Wrapper.sol:111:        if (!transferred || tokenAddress.code.length == 0) revert('TransferFailed()');

AxelarGateway.sol:504:            if (!burnSuccess) revert BurnFailed(symbol);
AxelarGateway.sol:515:            if (!burnSuccess) revert BurnFailed(symbol);

5. <array>.length should not be looked up in every loop of a for-loop

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

auth/AxelarAuthWeighted.sol:17:        for (uint256 i; i < recentOperators.length; ++i) {
auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
auth/AxelarAuthWeighted.sol:116:        for (uint256 i; i < accounts.length - 1; ++i) {
deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
gas-service/AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {

6. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

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

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

7. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

auth/AxelarAuthWeighted.sol:17:        for (uint256 i; i < recentOperators.length; ++i) {
auth/AxelarAuthWeighted.sol:69:        for (uint256 i = 0; i < weightsLength; ++i) {
auth/AxelarAuthWeighted.sol:98:        for (uint256 i = 0; i < signatures.length; ++i) {
auth/AxelarAuthWeighted.sol:101:            for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
auth/AxelarAuthWeighted.sol:116:        for (uint256 i; i < accounts.length - 1; ++i) {
deposit-service/AxelarDepositService.sol:114:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:168:        for (uint256 i; i < refundTokens.length; i++) {
deposit-service/AxelarDepositService.sol:204:        for (uint256 i; i < refundTokens.length; i++) {
gas-service/AxelarGasService.sol:123:        for (uint256 i; i < tokens.length; i++) {
AxelarGateway.sol:195:        for (uint256 i; i < adminCount; ++i) {
AxelarGateway.sol:207:        for (uint256 i = 0; i < symbols.length; i++) {
AxelarGateway.sol:292:        for (uint256 i; i < commandsLength; ++i) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

8. Upgrade pragma

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Contract existence checks (>= 0.8.10): external calls skip contract existence checks if the external call has a return value

Consider upgrading from 0.8.9 to at least 0.8.10 in the solution.

9. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

auth/AxelarAuthWeighted.sol:47:    function transferOperatorship(bytes calldata params) external onlyOwner {
gas-service/AxelarGasService.sol:120:    function collectFees(address payable receiver, address[] calldata tokens) external onlyOwner {
xc20/contracts/XC20Wrapper.sol:44:    function setXc20Codehash(bytes32 newCodehash) external onlyOwner {
xc20/contracts/XC20Wrapper.sol:66:    function removeWrapping(string calldata symbol) external onlyOwner {
AxelarGateway.sol:204:    function setTokenDailyMintLimits(string[] calldata symbols, uint256[] calldata limits) external override onlyAdmin {
AxelarGateway.sol:331:    function deployToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol:367:    function mintToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol:373:    function burnToken(bytes calldata params, bytes32) external onlySelf {
AxelarGateway.sol:397:    function approveContractCall(bytes calldata params, bytes32 commandId) external onlySelf {
AxelarGateway.sol:411:    function approveContractCallWithMint(bytes calldata params, bytes32 commandId) external onlySelf {
AxelarGateway.sol:437:    function transferOperatorship(bytes calldata newOperatorsData, bytes32) external onlySelf {