veToken Finance contest - FSchmoede's results

Lock more veAsset permanently.

General Information

Platform: Code4rena

Start Date: 26/05/2022

Pot Size: $75,000 USDT

Total HM: 31

Participants: 71

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 18

Id: 126

League: ETH

veToken Finance

Findings Distribution

Researcher Performance

Rank: 34/71

Findings: 3

Award: $255.64

🌟 Selected for report: 0

🚀 Solo Findings: 0

Findings Information

🌟 Selected for report: xiaoming90

Also found by: 0xNazgul, FSchmoede, Funen, Kumpa, VAD37, berndartmueller, cccz, kirk-baird

Labels

bug
duplicate
2 (Med Risk)
disagree with severity
sponsor disputed

Awards

99.6119 USDT - $99.61

External Links

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L219-L241

Vulnerability details

Missing limits for fees

In the method setFees() found in the contract Booster.sol there is no range limit for each individual fee type. There is a limit on the total maximum amount of fees, but not limits on how these should be distributed.

Therefore, it will be possible for the feeManager to set the platformFee to the maximum amount and set all other fees to 0.

Impact

By setting the platformFee to the maximum amount and set all other fees to 0 would mean that users would not earn any rewards for staking and locking their assets. Instead all rewards would be sent to the contracts treasury.

Since users don’t have any guarantee that this will never happen, they may feel discouraged to use the platform to stake veAssets, and hence maybe find another alternative, where they are sure, that the platform cannot keep all rewards in the future.

Proof of concept

The relevant code can be found at L219-L241

Add some ranges for each type of fee, just like in the original Convex code. There is even still the comment from the original Convex code about limit ranges, but the ranges themselves are removed (comment at L233). The method should re-implement the limits like:

function setFees(
        uint256 _lockFees,
        uint256 _stakerFees,
        uint256 _stakerLockIncentiveFee,
        uint256 _callerFees,
        uint256 _platform
    ) external {
        require(msg.sender == feeManager, "!auth");

        uint256 total = _lockFees.add(_stakerFees).add(_callerFees).add(_platform).add(
            _stakerLockIncentiveFee
        );
        require(total <= MaxFees, ">MaxFees");

        //values must be within certain ranges
        if(_lockFees >= 1000 && _lockFees <= 1500
            && _stakerFees >= 300 && _stakerFees <= 600
            && _callerFees >= 10 && _callerFees <= 100
            && _platform <= 200){
            lockIncentive = _lockFees;
            stakerIncentive = _stakerFees;
            stakerLockIncentive = _stakerLockIncentiveFee;
            earmarkIncentive = _callerFees;
	    platformFee = _platform;
            emit FeesUpdated(_lockFees, _stakerFees, _stakerLockIncentiveFee, _callerFees, _platform);
        }				
    }

#0 - jetbrain10

2022-06-15T16:29:06Z

I think check max fee is enough if we need it more flexible, and admin will be controlled by DAO and muti-sig wallet

#1 - GalloDaSballo

2022-07-25T00:27:37Z

Dup of #215

QA report

[L-01] Event emitted when nothing is updated

In the method setRewardContracts() in the contract Booster.sol the event RewardContractsUpdated is emitted even though nothing is updated.

The method is found at L174-L190:

function setRewardContracts(
    address _rewards,
    address _stakerRewards,
    address _stakerLockRewards
) external {
    require(msg.sender == owner, "!auth");

    //reward contracts are immutable or else the owner
    //has a means to redeploy and mint cvx via rewardClaimed()
    if (lockRewards == address(0)) {
        lockRewards = _rewards;
        stakerRewards = _stakerRewards;
        stakerLockRewards = _stakerLockRewards;
    }

    emit RewardContractsUpdated(_rewards, _stakerRewards, _stakerLockRewards);
}

The reward contracts are only updated if lockRewards == address(0), however the event is emitted every time the method is called, which is misleading. The emit of the event should be moved inside the if statement, so it is only emitted if the contracts are actually updated.

[L-02] Roles missing setter method

The roles operator and rewardManager in BaseRewardPool.sol and the role rewardManager in VE3DRewardPool.sol are immutables set in the constructors.

In the situation where the assigned roles needs to be changed, this can only be done by deploying new contracts, which could be circumvented by having appropriate setter methods, like there is for all the other roles in the remaining contracts.

[NC-01] Check for zero address

In methods where address to other contracts are assigned to a state variable, it is recommended to add in a zero address check to prevent human mistakes.

Examples of methods assigning addresses could be setFeeManager() and setPoolManager() in the contract Booster.sol.

This would mean that the method setFeeManager() found at L129-133 could look like this:

function setFeeManager(address _feeM) external {
    require(msg.sender == feeManager, "!auth");
    require(_feeM != address(0), "zero address")
    feeManager = _feeM;
    emit FeeManagerUpdated(_feeM);
}

#0 - GalloDaSballo

2022-07-07T22:42:56Z

[L-01] Event emitted when nothing is updated

Valid Refactoring

## [L-02] Roles missing setter method I don't agree with this being valid

[NC-01] Check for zero address

Valid Low

1L, 1R

Gas optimization report

[G-01] Optimize for loops

Several of the for-loops in the code can be optimized to cost less gas. One of the examples is the for-loop found in Booster.sol at L329-L340:

for (uint256 i = 0; i < poolInfo.length; i++) {
    PoolInfo storage pool = poolInfo[i];
    if (pool.shutdown) continue;

    address token = pool.lptoken;
    address gauge = pool.gauge;

    //withdraw from gauge
    try IStaker(staker).withdrawAll(token, gauge) {
        pool.shutdown = true;
    } catch {}
}

4 modifications can be done to this loop to save gas:

  1. There is no need to initialize the counter i to 0, since this is automatically done when declaring a uint.
  2. Preincrement ++i is cheaper than postincrement i++ , since the latter will make use of a temporary variable to store the returned value in before incrementing it. This can be saved by doing a preincrement, which increments the value and then returns it.
  3. The likelihood of a index inside a array is next to nothing, there is no need to use checked arithmetic, and this we can do unchecked arithmetic when incrementing the counter/array index i.
  4. Cache the array length in a local variable outside of the loop, so it will only be evaluated once. Otherwise the length of the array has to be calculated for every iteration of the loop which is more expensive than just reading the local variable.

With the above suggestions the for-loop above can be rewritten into:

uint256 arrayLength = poolInfo.length;
for (uint256 i; i < arrayLength;) {
    PoolInfo storage pool = poolInfo[i];
    if (pool.shutdown) continue;

    address token = pool.lptoken;
    address gauge = pool.gauge;

    //withdraw from gauge
    try IStaker(staker).withdrawAll(token, gauge) {
        pool.shutdown = true;
    } catch {}
	unchecked { ++i; }
}

[G-02] Redundant division and multiplication with constant

In the method _lockVeAsset() found in the contract VeAssetDepositor.sol the following code is found at L103:

uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;

The multiplication will equal out the division because it is done with the same constant WEEK, and hence the two operations can be removed to save gas. So essentially in this case unlockAt == (unlockAt / WEEK)* WEEK.

[G-03] Unneeded cache of variables in shutdownSystem()

In the method shutdownSystem() found in contract Booster.sol the state variables pool.lptoken and pool.gauge are cached in memory even though they are only used once. This means that instead of just having 2 SLOAD operations, there is now used 2 SLOAD, 2 MSTORE and 2 MLOAD operations. The two latter operations are basically just a waste of gas.

The code can be found at L333-L337:

address token = pool.lptoken; // SLOAD + MSTORE
address gauge = pool.gauge; // SLOAD + MSTORE

//withdraw from gauge
try IStaker(staker).withdrawAll(token, gauge) { // 2x MLOAD

This could be optimized to:

//withdraw from gauge
try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) { // 2x SLOAD

[G-04] Unneeded cache of variables in _withdraw()

In the method _withdraw() found in contract Booster.sol the state variables pool.lptoken and pool.gauge are cached in memory even though they are only used once. This means that instead of just having 2 SLOAD operations, there is now used 2 SLOAD, 2 MSTORE and 2 MLOAD operations. The two latter operations are basically just a waste of gas.

The code can be found at L402-L412:

address gauge = pool.gauge; // SLOAD + MSTORE

//remove lp balance
address token = pool.token; // SLOAD + MSTORE
ITokenMinter(token).burn(_from, _amount); // MLOAD

//pull from gauge if not shutdown
// if shutdown tokens will be in this contract
if (!pool.shutdown) {
    IStaker(staker).withdraw(lptoken, gauge, _amount); // MLOAD
}

This could be optimized to:

//remove lp balance
ITokenMinter(pool.token).burn(_from, _amount); // SLOAD

//pull from gauge if not shutdown
// if shutdown tokens will be in this contract
if (!pool.shutdown) {
    IStaker(staker).withdraw(lptoken, pool.gauge, _amount); // SLOAD
}

[G-05] Unneeded cache of variable in withdrawAll()

In the method withdrawAll() found in contract Booster.sol the state variables poolInfo[_pid].token is cached in memory even though it is only used once. This means that instead of just having 1 SLOAD operation, there is now used 1 SLOAD, 1 MSTORE and 1 MLOAD operation. The two latter operations are basically just a waste of gas.

The code can be found at L435-L436:

address token = poolInfo[_pid].token; // SLOAD + MSTORE
uint256 userBal = IERC20(token).balanceOf(msg.sender); // MLOAD

This could be optimized to:

uint256 userBal = IERC20(poolInfo[_pid].token).balanceOf(msg.sender); // SLOAD

[G-06] Unneeded cache of variable in withdrawTo()

In the method withdrawTo() found in contract Booster.sol the state variables poolInfo[_pid].veAssetRewards is cached in memory even though it is only used once. This means that instead of just having 1 SLOAD operation, there is now used 1 SLOAD, 1 MSTORE and 1 MLOAD operation. The two latter operations are basically just a waste of gas.

The code can be found at L447-L448:

address rewardContract = poolInfo[_pid].veAssetRewards; // SLOAD + MSTORE
require(msg.sender == rewardContract, "!auth"); // MLOAD

This could be optimized to:

require(msg.sender == poolInfo[_pid].veAssetRewards, "!auth"); // SLOAD

[G-07] Unneeded cache of variable in claimRewards()

In the method claimRewards() found in contract Booster.sol the state variables poolInfo[_pid].stash is cached in memory even though it is only used once. This means that instead of just having 1 SLOAD operation, there is now used 1 SLOAD, 1 MSTORE and 1 MLOAD operation. The two latter operations are basically just a waste of gas.

The code can be found at L476-477:

address stash = poolInfo[_pid].stash; // SLOAD + MSTORE
require(msg.sender == stash, "!auth"); // MLOAD

This could be optimized to:

require(msg.sender == poolInfo[_pid].stash, "!auth"); // SLOAD

[G-08] Unneeded cache of variable in setGaugeRedirect()

In the method setGaugeRedirect() found in contract Booster.sol the state variables poolInfo[_pid].gauge is cached in memory even though it is only used once. This means that instead of just having 1 SLOAD operation, there is now used 1 SLOAD, 1 MSTORE and 1 MLOAD operation. The two latter operations are basically just a waste of gas.

The code can be found at L486-L491:

address gauge = poolInfo[_pid].gauge; // SLOAD + MSTORE
bytes memory data = abi.encodeWithSelector(
    bytes4(keccak256("set_rewards_receiver(address)")),
    stash
);
IStaker(staker).execute(gauge, uint256(0), data); // MLOAD

This could be optimized to:

bytes memory data = abi.encodeWithSelector(
    bytes4(keccak256("set_rewards_receiver(address)")),
    stash
);
IStaker(staker).execute(poolInfo[_pid].gauge, uint256(0), data); // SLOAD

[G-09] Unneeded cache of variable in rewardClaimed()

In the method rewardClaimed() found in contract Booster.sol the state variables poolInfo[_pid].veAssetRewards is cached in memory even though it is only used once. This means that instead of just having 1 SLOAD operation, there is now used 1 SLOAD, 1 MSTORE and 1 MLOAD operation. The two latter operations are basically just a waste of gas.

The code can be found at L603-L604:

address rewardContract = poolInfo[_pid].veAssetRewards; // SLOAD + MSTORE
require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth"); // MLOAD

This could be optimized to:

require(msg.sender == poolInfo[_pid].veAssetRewards || msg.sender == lockRewards, "!auth"); // SLOAD

[G-10] Cache periodFinish

The state variable periodFinish in the contract BaseRewardPool.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L301-L312:

require(msg.sender == operator, "!authorized");

_rewards = _rewards.add(queuedRewards);

if (block.timestamp >= periodFinish) { // SLOAD1
    notifyRewardAmount(_rewards);
    queuedRewards = 0;
    return true;
}

//et = now - (finish-duration)
uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration)); // SLOAD2

As shown in the code example above, the state variable is read from storage twice, meaning two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

require(msg.sender == operator, "!authorized");

_rewards = _rewards.add(queuedRewards);
uint256 cachedPeriodFinish = periodFinish; // SLOAD + MSTORE

if (block.timestamp >= cachedPeriodFinish) { // MLOAD1
    notifyRewardAmount(_rewards);
    queuedRewards = 0;
    return true;
}

//et = now - (finish-duration)
uint256 elapsedTime = block.timestamp.sub(cachedPeriodFinish.sub(duration)); // MLOAD2

[G-11] Cache treasury

The state variable treasury in the contract Booster.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L526-L531:

if (treasury != address(0) && treasury != address(this) && platformFee > 0) { // SLOAD1
    //only subtract after address condition check
    uint256 _platform = veAssetBal.mul(platformFee).div(FEE_DENOMINATOR);
    veAssetBal = veAssetBal.sub(_platform);
    IERC20(veAsset).safeTransfer(treasury, _platform); // SLOAD2
}

As shown in the code example above, the state variable is read from storage twice, meaning two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedTreasury = treasury; // SLOAD + MSTORE
if (cachedTreasury != address(0) && cachedTreasury != address(this) && platformFee > 0) { // MLOAD1
    //only subtract after address condition check
    uint256 _platform = veAssetBal.mul(platformFee).div(FEE_DENOMINATOR);
    veAssetBal = veAssetBal.sub(_platform);
    IERC20(veAsset).safeTransfer(cachedTreasury, _platform); // MLOAD2
}

[G-12] Cache feeToken

The state variable feeToken in the contract Booster.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L578-L593:

IStaker(staker).claimFees(feeDistro, feeToken); // SLOAD1
//send fee rewards to reward contract
uint256 _balance = IERC20(feeToken).balanceOf(address(this)); // SLOAD2

uint256 _lockFeesIncentive = _balance.mul(lockFeesIncentive).div(FEE_DENOMINATOR);
uint256 _stakerLockFeesIncentive = _balance.mul(stakerLockFeesIncentive).div(
    FEE_DENOMINATOR
);
if (_lockFeesIncentive > 0) {
    IERC20(feeToken).safeTransfer(lockFees, _lockFeesIncentive); // SLOAD3
    IRewards(lockFees).queueNewRewards(_lockFeesIncentive);
}
if (_stakerLockFeesIncentive > 0) {
    IERC20(feeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive); // SLOAD4
    IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive); // SLOAD5
}

As shown in the code example above, the state variable is read from storage five times, meaning that in “worse” case we will have five gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 5 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 5 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedFeeToken = feeToken; // SLOAD + MSTORE
IStaker(staker).claimFees(feeDistro, cachedFeeToken); // MLOAD1
//send fee rewards to reward contract
uint256 _balance = IERC20(cachedFeeToken).balanceOf(address(this)); // MLOAD2

uint256 _lockFeesIncentive = _balance.mul(lockFeesIncentive).div(FEE_DENOMINATOR);
uint256 _stakerLockFeesIncentive = _balance.mul(stakerLockFeesIncentive).div(
    FEE_DENOMINATOR
);
if (_lockFeesIncentive > 0) {
    IERC20(cachedFeeToken).safeTransfer(lockFees, _lockFeesIncentive); // MLOAD3
    IRewards(lockFees).queueNewRewards(_lockFeesIncentive);
}
if (_stakerLockFeesIncentive > 0) {
    IERC20(cachedFeeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive); // MLOAD4
    IRewards(stakerLockRewards).queueNewRewards(cachedFeeToken, _stakerLockFeesIncentive); // MLOAD5
}

[G-13] Cache lockFees

The state variable lockFees in the contract Booster.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L587-L588:

IERC20(feeToken).safeTransfer(lockFees, _lockFeesIncentive); // SLOAD1
IRewards(lockFees).queueNewRewards(_lockFeesIncentive); // SLOAD2

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedLockFees = lockFees; // SLOAD + MSTORE
IERC20(feeToken).safeTransfer(cachedLockFees, _lockFeesIncentive); // MLOAD1
IRewards(cachedLockFees).queueNewRewards(_lockFeesIncentive); // MLOAD2

[G-14] Cache stakerLockRewards

The state variable stakerLockRewards in the contract Booster.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L591-L592:

IERC20(feeToken).safeTransfer(stakerLockRewards, _stakerLockFeesIncentive); // SLOAD1
IRewards(stakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive); // SLOAD2

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedStakerLockRewards; // SLOAD + MSTORE
IERC20(feeToken).safeTransfer(cachedStakerLockRewards, _stakerLockFeesIncentive); // MLOAD1
IRewards(cachedStakerLockRewards).queueNewRewards(feeToken, _stakerLockFeesIncentive); // MLOAD2

[G-15] Cache incentiveVeAsset in lockVeAsset()

The state variable incentiveVeAsset in the contract VeAssetDepositor.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L117-L120:

if (incentiveVeAsset > 0) { // SLOAD1
    ITokenMinter(minter).mint(msg.sender, incentiveVeAsset); // SLOAD2
    incentiveVeAsset = 0;
}

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

uint256 cachedIncentiveVeAsset = incentiveVeAsset; // SLOAD + MSTORE
if (cachedIncentiveVeAsset > 0) { // MLOAD1
    ITokenMinter(minter).mint(msg.sender, cachedIncentiveVeAsset); // MLOAD2
    incentiveVeAsset = 0;
}

[G-16] Cache incentiveVeAsset in deposit()

The state variable incentiveVeAsset in the contract VeAssetDepositor.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L138-L142:

if (incentiveVeAsset > 0) { // SLOAD1
    //add the incentive tokens here so they can be staked together
    _amount = _amount.add(incentiveVeAsset); // SLOAD2
    incentiveVeAsset = 0;
}

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

uint256 cachedIncentiveVeAsset = incentiveVeAsset; // SLOAD + MSTORE
if (cachedIncentiveVeAsset > 0) { // MLOAD1
    //add the incentive tokens here so they can be staked together
    _amount = _amount.add(cachedIncentiveVeAsset); // MLOAD2
    incentiveVeAsset = 0;
}

[G-17] Cache totalCliffs

The state variable totalCliffs in the contract VeTokenMinter.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L59-L74:

if (cliff < totalCliffs) { // SLOAD1
    //for reduction% take inverse of current cliff
    uint256 reduction = totalCliffs.sub(cliff); // SLOAD2
    //reduce
    _amount = _amount.mul(reduction).div(totalCliffs); // SLOAD3

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

    //mint
    veToken.safeTransfer(_to, _amount);
    totalSupply += _amount;
}

As shown in the code example above, the state variable is read from storage two times, meaning that we will have three gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 3 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 3 MLOAD operations, which is cheaper.

This means the code can be changed into:

uint256 cachedTotalCliffs = totalCliffs; // SLOAD + MSTORE
if (cliff < cachedTotalCliffs) { // MLOAD1
    //for reduction% take inverse of current cliff
    uint256 reduction = cachedTotalCliffs.sub(cliff); // MLOAD2
    //reduce
    _amount = _amount.mul(reduction).div(cachedTotalCliffs); // MLOAD3

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

    //mint
    veToken.safeTransfer(_to, _amount);
    totalSupply += _amount;
}

[G-18] Cache operator in claimVeAsset()

The state variable operator in the contract VoterProxy.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L225-L251:

require(msg.sender == operator, "!auth"); // SLOAD1

...

_balance = IERC20(veAsset).balanceOf(address(this));
IERC20(veAsset).safeTransfer(operator, _balance); // SLOAD2

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedOperator = operator; // SLOAD + MSTORE
require(msg.sender == cachedOperator, "!auth"); // MLOAD1

...

_balance = IERC20(veAsset).balanceOf(address(this));
IERC20(veAsset).safeTransfer(cachedOperator, _balance); // MLOAD2

[G-19] Cache operator in claimFees()

The state variable operator in the contract VoterProxy.sol can be cached to save gas on the expensive SLOAD operations.

The code can be found at L263-L266:

require(msg.sender == operator, "!auth"); // SLOAD1
IFeeDistro(_distroContract).claim();
uint256 _balance = IERC20(_token).balanceOf(address(this));
IERC20(_token).safeTransfer(operator, _balance); // SLOAD2

As shown in the code example above, the state variable is read from storage two times, meaning that we will have two gas expensive SLOAD operations. Instead, we can cache the variable in memory, such that the 2 SLOAD operations will be converted into 1 SLOAD, 1 MSTORE and 2 MLOAD operations, which is cheaper.

This means the code can be changed into:

address cachedOperator = operator; // SLOAD + MSTORE
require(msg.sender == cachedOperator, "!auth"); // MLOAD1
IFeeDistro(_distroContract).claim();
uint256 _balance = IERC20(_token).balanceOf(address(this));
IERC20(_token).safeTransfer(cachedOperator, _balance); // MLOAD2

[G-20] Make maxTime immutable

The state variable maxTime in the contract VeAssetDepositor.sol is only set in the constructor. Hence there is no reason for this to not an immutable to save some gas.

The variable can be found at L30:

uint256 private maxTime;

And could be changed to:

uint256 private immutable MAX_TIME;

[G-21] Don’t compare boolean to constant

It is more expensive to compare a boolean to a constant true/false than just evaluate the boolean it self, and both options will give the same result. Therefore, do not use == true or == false in requireor if statements.

Examples of this can be found in the method deposit() in VoterProxy.sol at L93-L98:

if (protectedTokens[_token] == false) {
    protectedTokens[_token] = true;
}
if (protectedTokens[_gauge] == false) {
    protectedTokens[_gauge] = true;
}

Can be changed into:

if (!protectedTokens[_token]) {
    protectedTokens[_token] = true;
}
if (!protectedTokens[_gauge]) {
    protectedTokens[_gauge] = true;
}

#0 - GalloDaSballo

2022-07-18T23:24:53Z

Should save 2.1k (immutable) + another 2 or 3k

Let's say roughly 5.1k saved

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter