Gas Optimizations - Githubissues

code423n4 commented 2 years ago

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

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

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) {

re1ro commented 2 years ago

Minimize the Number of SLOADs by Caching State Variable

Invalid. It's read and write

Defined Variables Used Only Once

gatewayToken should be moved out of the loop

Use Calldata instead of Memory for Read Only Function Parameters

Dup #7

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

Dup #12

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

Dup #7

Unnecessary Default Value Initialization

Dup #2

++i Costs Less Gas than i++

Dup #2

!= 0 costs less gass than > 0

Dup #17

GalloDaSballo commented 2 years ago

Minimize the Number of SLOADs by Caching State Variable

Giving it 100 as sign of appreciation

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

This has been debunked for ages https://twitter.com/GalloDaSballo/status/1543729080926871557

Calldata instead of Memory for Read Only Function Param

60 for the array of bytes, for the rest pls provide benchmark next time

Rest is usual loop stuff, 300 gas

460

GalloDaSballo commented 2 years ago

Really appreciated the custom finding, the rest is not impressive as other 50 submissions sent the same advice, recommend focusing on unique findings to get ahead

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

Gas Optimizations #181

Table of Contents

Minimize the Number of SLOADs by Caching State Variable

Issue

PoC

Mitigation

Defined Variables Used Only Once

Issue

PoC

Mitigation

Use Calldata instead of Memory for Read Only Function Parameters

Issue

PoC

Mitigation

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

Issue

PoC

Mitigation

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

Issue

PoC

Mitigation

Unnecessary Default Value Initialization

Issue

PoC

Mitigation

++i Costs Less Gas than i++

Issue

PoC

Mitigation

Minimize the Number of SLOADs by Caching State Variable

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

Calldata instead of Memory for Read Only Function Param