code-423n4 / 2022-07-axelar-findings

0 stars 0 forks source link

Gas Optimizations #211

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Overview

Risk Rating Number of issues
Gas Issues 9

Table of Contents:

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:

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];
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.

  30:         IERC20(wrappedTokenAddress).approve(gateway, amount); //@audit gas SLOAD (gateway)
  31:         IAxelarGateway(gateway).sendToken(destinationChain, destinationAddress, wrappedSymbol(), amount); //@audit gas SLOAD (gateway)
  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)
  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:

Decrement:

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:

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 {
GalloDaSballo commented 2 years ago

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

I seem to be getting between 20 and 40 gas savings in caching, I can't quite explain why this would happen for calldata. For storage the position is recomputed, and that takes a keccak (30 gas) + costs for handling memory, but for calldata it's just an offset (I'd assume 3 gas) + calldata load (up to 12 gas on evm.codes)

Will give it 20 gas per instance per calldata * 2 = 40

40 for Storage * 5 = 200

3. Caching Storage into Memory

First time saves 94 gas (100 - 6 - SLOAD + MLOAD), each subsequent is 97 (100 - 3 - SLOAD)

94+94+97+94

5 + 6 + 7

Around 300 gas like other submissions

Upgrade pragma

Skipping external calls will save about 100 gas per instance, however no instances were listed, giving it 100 gas

In this case I don't think 1 is applicable, same for the last function as while it does save gas it's a trade-off

All in all way better than average report, highly recommend the sponsor to follow the advice

1019 gas saved