search

code-423n4 / 2022-02-concur-findings

2 stars 0 forks source link

Gas Optimizations #185

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Issue #1: Declare variables outside of for loop to save gas

Impact

Declaring variables inside for loops caused these variables to be re-declared in every loop. You can save gas by declaring them outside the for loop and updating them inside.

Where in the code

Steps to Mitigate

Using the example in the first link, simply change:

for (uint256 i = 0; i < extraCount; i++) {
  address extraPool = IRewardStaking(mainPool).extraRewards(i);
  address extraToken = IRewardStaking(extraPool).rewardToken();
    //...rest of the code

to

address extraPool;
address extraToken;
for (uint256 i = 0; i < extraCount; i++) {
  extraPool = IRewardStaking(mainPool).extraRewards(i);
  extraToken = IRewardStaking(extraPool).rewardToken();
    //...rest of the code

Proof of Concept

I made some mock tests in foundry to check the difference in gas.

The results:

Here is the example code, bare in mind I separate these in different files to make sure there’s no extra gas added due to how the dispatcher finds functions:

Contract A

pragma solidity ^0.8.4;

contract ForLoopA {

    function forLoopA() public pure {
        for (uint256 i = 0; i < 100; i++) {
            uint256 someVariable = i;
        }
    }
}

Contract B

pragma solidity ^0.8.4;

contract ForLoopB {

    function forLoopB() public pure {
        uint256 someVariable;
        for (uint256 i = 0; i < 100; i++) {
            someVariable = i;
        }
    }
}

Test

pragma solidity ^0.8.4;

import "ds-test/test.sol";
import "../ForLoopA.sol";
import "../ForLoopB.sol";

contract TestForLoop is DSTest {
    ForLoopA a;
    ForLoopB b;
    function setUp() public {
        a = new ForLoopA();
        b = new ForLoopB();
  }

  function testA() public logs_gas {
        a.forLoopA();
    }
  function testB() public logs_gas {
        b.forLoopB();
    }
}

Issue #2: Unnecessary safeApprove update to 0

Impact

This line may be redundant in the logic of the deposit function. If I’m not missing something, you are approving lpToken to spend _amount, then that _amount is deposited into the convexBooster, which should set the allowances to _amount - _amount, which is 0, so the highlighted line where the approve is updated to 0 seems redundant, so it could be removed to save some gas and contract size. Again, I could be missing some special behaviour here.

Steps to mitigate

Remove this line.

Issue #3: Cast mapping into storage variable inside function to save gas

Impact

Declaring state variables used more than once in the logic of a function can save gas. In this case, the deposit and withdraw functions can cast deposits[_pid][msg.sender] into a storage variable to save gas.

Where in the Code

Steps to mitigate

Create a storage variable inside the function and use that variable instead of the mapping.

Using deposit as an example, change:

function deposit(uint256 _pid, uint256 _amount)
        external
        whenNotPaused
        nonReentrant
    {
        _checkpoint(_pid, msg.sender);
        deposits[_pid][msg.sender].epoch = currentEpoch();
        deposits[_pid][msg.sender].amount += uint192(_amount);
                ///... rest of the code

to: 

function deposit(uint256 _pid, uint256 _amount)
        external
        whenNotPaused
        nonReentrant
    {
        _checkpoint(_pid, msg.sender);
                Deposit storage _deposits = deposits[_pid][msg.sender]
        deposits.epoch = currentEpoch();
        deposits.amount += uint192(_amount);
                ///... rest of the code

Proof of Concept

I ran mock gas-tests using foundry. The results:

Here are the contracts I ran along with the tests. Bare in mind I separate the logic in different contracts to avoid the extra gas from the binary search the dispatcher does to find the function to call.

Contract A

pragma solidity ^0.8.4;

contract MappingStructA {

    struct Deposit {
        uint64 epoch;
        uint192 amount;
    }

    mapping(uint256 => mapping(address => Deposit)) public deposits;

    function declarationA() public {
        deposits[10][address(0)].epoch += 100; 
        deposits[10][address(0)].amount += 100; 
    }
}

Contract B

pragma solidity ^0.8.4;

contract MappingStructB {

    struct Deposit {
        uint64 epoch;
        uint192 amount;
    }

    mapping(uint256 => mapping(address => Deposit)) public deposits;

    function declarationB() public {
        Deposit storage _deposits = deposits[10][address(0)];
        _deposits.epoch += 100; 
        _deposits.amount += 100; 
    }
}

Test

pragma solidity ^0.8.4;

import "ds-test/test.sol";
import "../MappingStructA.sol";
import "../MappingStructB.sol";

contract TestMappingStruct is DSTest {
    MappingStructA a;
    MappingStructB b;
    function setUp() public {
        a = new MappingStructA();
        b = new MappingStructB();
  }

  function testA() public logs_gas {
        a.declarationA();
    }
  function testB() public logs_gas {
        b.declarationB();
    }
}

Issue #4: Move checks at the beginning of the function to save gas on reverting calls

Impact

It’s always a good pattern to put checks as the first thing in a function to make sure if something’s wrong, then there’s no extra logic computed, therefore there’s gas saved in a revertir of failing call.

Where in the code

Steps to Mitigate

Change:

To:

function add(address _token, uint _allocationPoints, uint16 _depositFee, uint _startBlock) public onlyOwner {
    require(_token != address(0), "zero address");
        require(pid[_token] == 0, "already registered"); // pid starts from 0
    uint lastRewardBlock = block.number > _startBlock ? block.number : _startBlock;
    totalAllocPoint = totalAllocPoint.add(_allocationPoints);
    //...rest of the code

Change:

To:

function withdraw(address _recipient, uint _pid, uint _amount) external nonReentrant onlyDepositor {
    UserInfo storage user = userInfo[_pid][_msgSender()];
    require(user.amount > 0, "MasterChef: nothing to withdraw");
    require(user.amount >= _amount, "MasterChef: withdraw not allowed");
    PoolInfo storage pool = poolInfo[_pid];
        updatePool(_pid);

Issue #5: Remove poolLength() function

Impact

The poolLength() function is never called and adds unnecessary code size. Because poolInfo is a public variable, getting is length either on-chain or off-chain is trivial. One can call the contract.poolInfo() and then compute the length of the result to get the length. This function can be removed.

Steps to Mitigate

Remove poolLength() function.

Issue #6: Shorten error messages

Impact

This is a common gas optimization possibility that appears in a lot of contracts. For this reason, wardens have added it to a list of common gas optimizations. I’ll link to the explanation, which is as detailed as it gets:

https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g007---long-revert-strings

In short, if you are using Solidity version >=0.8.4, the most gas-efficient approach is to create custom errors and use if (condition) revert customError() syntax. The second best approach is to shorten your error strings to have a length less than 32. Here’s where you can apply those optimization in your error messages:

Issue #7: Make step immutable or constant

Impact

In USDMPegRecovery the state variable step is public. However, it doesn’t have a setter, it’s value is not updated throughout the contract, and its value is set to a constant number value in the constructor. Immutable and constant state variables are cheaper than internal function.

Steps to mitigate

Change:

uint256 public step;

to

uint256 public constant step;
//or
uint256 public immutable step;
GalloDaSballo commented 2 years ago

-1 Declare variables outside of for loop to save gas Crazy how the warden has proof to show gas savings, am pretty confident there would be no savings with optimizer. As the savings make no sense. Technically the suggested version is declaring 2 extra mstores (MSTORE -> 0 in the variable) however they proven to save gas so that's 537 gas

-2 Unnecssary approve 0

Per this line: https://github.com/convex-eth/platform/blob/3ea150f5bf5482cb0572b67cd92ecce3b345249d/contracts/contracts/Booster.sol#L257

I agree with the unnecessary approve 0, per evm.codes this is 100 gas saved

Screenshot 2022-03-30 at 02 10 46

Let's say this would save 50 gas

For sake of simplicity let's assume that each character is one byte

That means that for each error message above 32 bytes you're spending an extra char * 2500 gas to store that onChain.

While this is still a deployment gas saving, I'll give it 2500 gas savings score as it effectively saves a ton of gas. 2500 * 4 = 10000

Total 10687 gas saved

GalloDaSballo commented 2 years ago

Also want to commend the warden for the amazing formatting and thoroughness of the report