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

0 stars 0 forks source link

Gas Optimizations #174

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be changed to:

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

Consider declaring the following lines without explicitly setting the index to 0:

contracts/AxelarGateway.sol:
 207:        for (uint256 i = 0; i < symbols.length; i++) {

contracts/auth/AxelarAuthWeighted.sol:
  69:        for (uint256 i = 0; i < weightsLength; ++i) {
  98:        for (uint256 i = 0; i < signatures.length; ++i) {

For-Loops: Cache array length outside of loops

Reading an array's length at each iteration has the following gas overheads:

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset. This would save around 3 gas per iteration.

For example:

for (uint256 i; i < arr.length; ++i) {}

can be changed to:

uint256 len = arr.length;
for (uint256 i; i < len; ++i) {}

Consider making the following change to these lines:

contracts/AxelarGateway.sol:
 207:        for (uint256 i = 0; i < symbols.length; i++) {

contracts/deposit-service/AxelarDepositService.sol:
 114:        for (uint256 i; i < refundTokens.length; i++) {
 168:        for (uint256 i; i < refundTokens.length; i++) {
 204:        for (uint256 i; i < refundTokens.length; i++) {

contracts/auth/AxelarAuthWeighted.sol:
  17:        for (uint256 i; i < recentOperators.length; ++i) {
  98:        for (uint256 i = 0; i < signatures.length; ++i) {

contracts/gas-service/AxelarGasService.sol:
 123:        for (uint256 i; i < tokens.length; i++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, index increments can be left unchecked to save 30-40 gas per loop iteration.

For example, the code below:

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

can be changed to:

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

Consider making the following change to these lines:

contracts/AxelarGateway.sol:
 195:        for (uint256 i; i < adminCount; ++i) {
 207:        for (uint256 i = 0; i < symbols.length; i++) {
 292:        for (uint256 i; i < commandsLength; ++i) {

contracts/deposit-service/AxelarDepositService.sol:
 114:        for (uint256 i; i < refundTokens.length; i++) {
 168:        for (uint256 i; i < refundTokens.length; i++) {
 204:        for (uint256 i; i < refundTokens.length; i++) {

contracts/auth/AxelarAuthWeighted.sol:
  17:        for (uint256 i; i < recentOperators.length; ++i) {
  69:        for (uint256 i = 0; i < weightsLength; ++i) {
  98:        for (uint256 i = 0; i < signatures.length; ++i) {
 101:        for (; operatorIndex < operatorsLength && signer != operators[operatorIndex]; ++operatorIndex) {}
 116:        for (uint256 i; i < accounts.length - 1; ++i) {

contracts/gas-service/AxelarGasService.sol:
 123:        for (uint256 i; i < tokens.length; i++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/AxelarGateway.sol:
 207:        for (uint256 i = 0; i < symbols.length; i++) {

contracts/deposit-service/AxelarDepositService.sol:
 114:        for (uint256 i; i < refundTokens.length; i++) {
 168:        for (uint256 i; i < refundTokens.length; i++) {
 204:        for (uint256 i; i < refundTokens.length; i++) {

contracts/gas-service/AxelarGasService.sol:
 123:        for (uint256 i; i < tokens.length; i++) {

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

xc20/contracts/XC20Wrapper.sol:
  40:        function contractId() public pure returns (bytes32) {

contracts/deposit-service/AxelarDepositService.sol:
 241:        function contractId() public pure returns (bytes32) {

contracts/deposit-service/DepositBase.sol:
  41:        function wrappedToken() public view returns (address) {

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/auth/AxelarAuthWeighted.sol:
  68:        uint256 totalWeight = 0;
  94:        uint256 operatorIndex = 0;
  95:        uint256 weight = 0;

Use calldata instead of memory for read-only arguments in external functions

When an external function with a memory array is called, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length).

Using calldata directly helps to save gas as values are read directly from calldata using calldataload, thus removing the need for such a loop in the contract code during runtime execution.

Also, structs have the same overhead as an array of length one.

Consider changing the following from memory to calldata:

xc20/contracts/XC20Wrapper.sol:
  51:        string memory newName,
  52:        string memory newSymbol

contracts/AxelarGateway.sol:
 447:        function _unpackLegacyCommands(bytes memory executeData)

contracts/gas-service/AxelarGasService.sol:
  40:        string memory symbol,

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

As seen from here:

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

However, this does not apply to storage values as using reduced-size types might be beneficial to pack multiple elements into a single storage slot. Thus, where appropriate, use uint256/int256 and downcast when needed.

Consider using uint256/int256 instead of bool for the following:

contracts/AxelarGateway.sol:
 332:        (string memory name, string memory symbol, uint8 decimals, uint256 cap, address tokenAddress, uint256 dailyMintLimit) = abi.decode(

abi.encode() is less efficient than abi.encodePacked()

Instances where abi.encodePacked() should be used rather than abi.encode():

contracts/deposit-service/AxelarDepositService.sol:
 233:        keccak256(abi.encodePacked(type(DepositReceiver).creationCode, abi.encode(delegateData)))

internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Consider inlining the following internal functions:

xc20/contracts/XC20Wrapper.sol:
 101:        function _safeTransferFrom(
 102:            address tokenAddress,
 103:            address from,
 104:            uint256 amount
 105:        ) internal {

contracts/AxelarGateway.sol:
 611:        function _setTokenDailyMintAmount(string memory symbol, uint256 amount) internal {
 622:        function _setTokenAddress(string memory symbol, address tokenAddress) internal {

 630:        function _setContractCallApproved(
 631:            bytes32 commandId,
 632:            string memory sourceChain,
 633:            string memory sourceAddress,
 634:            address contractAddress,
 635:            bytes32 payloadHash
 636:        ) internal {

 640:        function _setContractCallApprovedWithMint(
 641:            bytes32 commandId,
 642:            string memory sourceChain,
 643:            string memory sourceAddress,
 644:            address contractAddress,
 645:            bytes32 payloadHash,
 646:            string memory symbol,
 647:            uint256 amount
 648:        ) internal {

 655:        function _setImplementation(address newImplementation) internal {

contracts/auth/AxelarAuthWeighted.sol:
  86:        function _validateSignatures(
  87:            bytes32 messageHash,
  88:            address[] memory operators,
  89:            uint256[] memory weights,
  90:            uint256 threshold,
  91:            bytes[] memory signatures
  92:        ) internal pure {

 115:        function _isSortedAscAndContainsNoDuplicate(address[] memory accounts) internal pure returns (bool) {

keccak256() should only need to be called on a specific string literal once

The result of keccak256() should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once.

Instances of keccak256() that can be saved to an immutable variable:

xc20/contracts/XC20Wrapper.sol:
  41:        return keccak256('xc20-wrapper');

contracts/deposit-service/AxelarDepositService.sol:
 242:        return keccak256('axelar-deposit-service');

contracts/deposit-service/AxelarDepositServiceProxy.sol:
   9:        return keccak256('axelar-deposit-service');

contracts/gas-service/AxelarGasServiceProxy.sol:
  10:        return keccak256('axelar-gas-service');

contracts/gas-service/AxelarGasService.sol:
 181:        return keccak256('axelar-gas-service');
re1ro commented 2 years ago

Dup #3 #7 #64

GalloDaSballo commented 2 years ago

keccak256() should only need to be called on a specific string literal once

30 per instance 150

Rest will save less than 300 gas

450