search

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

2 stars 0 forks source link

Gas Optimizations #91

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Concur Finance Gas Optimization Report

Unless otherwise noted, manual auditing and testing were done using Visual Studio Code and Remix. If the sponsor's test suite was provided then that suite was also used to verify the findings.

The audit was done from February 3-7, 2022 by ye0lde through code4rena.

Findings

G-1 - updatepool can be refactored to remove redundant code (MasterChef.sol)

Impact

Unneeded branching and redundant code can be removed to save gas.

Proof of Concept

updatePool is here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L134-L154

// Update reward variables of the given pool to be up-to-date.
function updatePool(uint _pid) public {
    PoolInfo storage pool = poolInfo[_pid];
    if (block.number <= pool.lastRewardBlock) {
        return;
    }
    uint lpSupply = pool.depositToken.balanceOf(address(this));
    if (lpSupply == 0 || pool.allocPoint == 0) {
        pool.lastRewardBlock = block.number;
        return;
    }
    if(block.number >= endBlock) {
        pool.lastRewardBlock = block.number;
        return;
    }        

    uint multiplier = getMultiplier(pool.lastRewardBlock, block.number);
    uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
    pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
    pool.lastRewardBlock = block.number;
}

Recommended Mitigation Steps

I recommend refactoring the function to this:

// Update reward variables of the given pool to be up-to-date.
function updatePool(uint _pid) public {
    PoolInfo storage pool = poolInfo[_pid];
    if (block.number <= pool.lastRewardBlock) {
        return;
    }
    uint lpSupply = pool.depositToken.balanceOf(address(this));
    if (!(lpSupply == 0 || pool.allocPoint == 0 || block.number >= endBlock)) {
        uint multiplier = getMultiplier(pool.lastRewardBlock, block.number);
        uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
        pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
    }
    pool.lastRewardBlock = block.number;
}

G-2 - ConvertCrvToCvx can be refactored to be more efficient (CvxMining.sol)

Impact

Unneeded caching of state variables and reduced branching can save gas.

Proof of Concept

ConvertCrvToCvx is here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/library/CvxMining.sol#L10-L34

function ConvertCrvToCvx(uint256 _amount) external view returns(uint256){
    uint256 supply = cvx.totalSupply();
    uint256 reductionPerCliff = cvx.reductionPerCliff();
    uint256 totalCliffs = cvx.totalCliffs();
    uint256 maxSupply = cvx.maxSupply();

    uint256 cliff = supply / reductionPerCliff;
    //mint if below total cliffs
    if(cliff < totalCliffs){
        //for reduction% take inverse of current cliff
        uint256 reduction = totalCliffs - cliff;
        //reduce
        _amount = _amount * reduction / totalCliffs;

        //supply cap check
        uint256 amtTillMax = maxSupply - supply;
        if(_amount > amtTillMax){
            _amount = amtTillMax;
        }

        //mint
        return _amount;
    }
    return 0;
}

Recommended Mitigation Steps

I recommend refactoring the function to this:

function ConvertCrvToCvx(uint256 _amount) external view returns(uint256){
    uint256 supply = cvx.totalSupply();
    uint256 totalCliffs = cvx.totalCliffs(); 

    uint256 cliff = supply / cvx.reductionPerCliff();
    //mint if below total cliffs
    if(cliff < totalCliffs){
        //for reduction% take inverse of current cliff
        uint256 reduction = totalCliffs - cliff;
        //reduce
        _amount = _amount * reduction / totalCliffs;

        //supply cap check  
        uint256 amtTillMax =  cvx.maxSupply() - supply;
        // mint
        return _amount > amtTillMax ? amtTillMax : _amount;           
    }
    return 0;
}

G-3 - safeConcurTransfer can be refactored to be more efficient (MasterChef.sol)

Impact

Unneeded variables and function calls can be removed to save gas.

Proof of Concept

safeConcurTransfer is here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L201-L211

// Safe [concur] transfer function, just in case if rounding error causes pool to not have enough
function safeConcurTransfer(address _to, uint _amount) private {
    uint concurBalance = concur.balanceOf(address(this));
    bool transferSuccess = false;
    if (_amount > concurBalance) {
        transferSuccess = concur.transfer(_to, concurBalance);
    } else {
        transferSuccess = concur.transfer(_to, _amount);
    }
    require(transferSuccess, "safeConcurTransfer: transfer failed");
}

Recommended Mitigation Steps

I recommend changing the function to this:

// Safe [concur] transfer function, just in case if rounding error causes pool to not have enough
function safeConcurTransfer(address _to, uint _amount) private {  
    uint concurBalance = concur.balanceOf(address(this));
    require(concur.transfer(_to, _amount > concurBalance ? concurBalance : _amount), 
            "safeConcurTransfer: transfer failed"); 
}

G-4 - Intermediate variables can be deleted in notifyRewardAmount (StakingAwards.sol)

Impact

Several intermediate variables are used only once and can be deleted to reduce gas usage.

Proof of Concept

The variable remaining is used only once and can be deleted: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L145-L146

The variable balance is used only once and can be deleted: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L154-L156

Recommended Mitigation Steps

I reccommend changing the function to this:

function notifyRewardAmount(uint256 reward)
    external
    updateReward(address(0))
{
    require(
        msg.sender == rewardsDistribution,
        "Caller is not RewardsDistribution contract"  
    );

    if (block.timestamp >= periodFinish) {
        rewardRate = reward / rewardsDuration;
    } else {
        uint256 leftover = (periodFinish - block.timestamp) * rewardRate;
        rewardRate = (reward + leftover) / rewardsDuration;
    }

    // Ensure the provided reward amount is not more than the balance in the contract.
    // This keeps the reward rate in the right range, preventing overflows due to
    // very high values of rewardRate in the earned and rewardsPerToken functions;
    // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
    require(
        rewardRate <= rewardsToken.balanceOf(address(this)) / rewardsDuration,  
        "Provided reward too high"
    );

    lastUpdateTime = block.timestamp;
    periodFinish = block.timestamp + rewardsDuration;
    emit RewardAdded(reward);
}

G-5 - SafeMath is not needed (MasterChef.sol)

Impact

The MasterChef.sol contract uses Solidity version 0.8.11 which already implements overflow checks by default. At the same time, it uses the SafeMath library which uses more gas than the 0.8.11 built-in support.

It should just use the built-in support and remove SafeMath from the dependencies.

Proof of Concept

SafeMath is imported here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L10

Computations using Safemath: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L89 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L109 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L120-L121 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L151-L152 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L171

Recommended Mitigation Steps

Remove SafeMath per the POC

G-6 - Checking for non-zero transfer values

Impact

Checking non-zero transfer values can avoid an external call to save gas. Checking if amount > 0 before making the external call to transfer can save gas by avoiding the external call.

Proof of Concept

Transfers values that could be checked are here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L57 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L37

Recommended Mitigation Steps

Check for non-zero transfer value before doing transfer.

G-7 - Called functions can be in-lined in _checkpoint (ConvexStakingWrapper.sol)

Impact

To save gas you can get rid of _getDepositedBalance and getTotalSupply and move their code (one line each) into _checkpoint. These two functions are internal and are only called once so there is no need for reusability and removing these extra calls will make execution cheaper.

Proof of Concept

_getDepositedBalance and getTotalSupply are here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L146-L162

_checkpoint is here; https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L209-L222

Recommended Mitigation Steps

I recommend refactoring the _checkpoint function to this:

function _checkpoint(uint256 _pid, address _account) internal {
    //if shutdown, no longer checkpoint in case there are problems
    if (paused()) return;

    uint256 supply = IRewardStaking(convexPool[_pid]).balanceOf(address(this));
    uint256 depositedBalance = deposits[_pid][_account].amount;

    IRewardStaking(convexPool[_pid]).getReward(address(this), true);

    uint256 rewardCount = rewards[_pid].length;
    for (uint256 i = 0; i < rewardCount; i++) {
        _calcRewardIntegral(_pid, i, _account, depositedBalance, supply);
    }
}

G-8 - Assignment Of Variables To Default

Impact

Variables are being assigned their default value which is unnecessary. Removing the assignment will save gas when deploying.

Proof of Concept

Variables are being assigned their default value in the following locations: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L51 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L21-L22

Recommended Mitigation Steps

Remove the assignments.

And if you feel it is important to show the default assignment will occur then replace the assignments with a comment.

G-9 - Long Revert Strings

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

Revert strings > 32 bytes are here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L210

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L139 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L172 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L181 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L193

Recommended Mitigation Steps

Consider shortening the revert strings to fit in 32 bytes or using custom errors in the future.

G-10 - Comparison with literal boolean value (VoteProxy.sol)

Impact

Gas savings and code clarity. While this is considered a "best practice" it also does save a small amount of gas during deployment and execution. ~85g/12g in this case.

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/VoteProxy.sol#L21

Recommended Mitigation Steps

Use the compared expression itself in place of comparison with a boolean literal. The expression can be replaced as is when the expression is expected to evaluate to true and negation can be used when the expression is expected to have a false value.

GalloDaSballo commented 2 years ago

G-1 - updatepool can be refactored to remove redundant code (MasterChef.sol)

Nice refactoring that would save gas only in the "unhappy cases"

G-2 - ConvertCrvToCvx can be refactored to be more efficient (CvxMining.sol) This will save 6 gas per variable removed (3 MSTORE and 3 MLOAD) 12

G-3 - safeConcurTransfer can be refactored to be more efficient (MasterChef.sol) I don't believe the ternary to save gas, however the check on the require does 6

G-4 - Intermediate variables can be deleted in notifyRewardAmount (StakingAwards.sol) 12 gas (6 * 2)

G-5 - SafeMath is not needed (MasterChef.sol) Each removal should save about 20 gas, 20 * 5 = 100

G-6 - Checking for non-zero transfer values Hard to quantify savings, but valid finding

G-7 - Called functions can be in-lined in _checkpoint (ConvexStakingWrapper.sol) Would save 8 gas per function, 8 * 3 = 24

G-8 - Assignment Of Variables To Default 3 * 100 = 300

G-9 - Long Revert Strings 2500 per the discussion in https://github.com/code-423n4/2022-02-concur-findings/issues/185

10000

G-10 - Comparison with literal boolean value (VoteProxy.sol) Not sure how the warden came up with 85 / 12 g as the comparison should cost 3 extra gas (EQ)

Total Gas saved 10457