veToken Finance contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L305-L329 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L349-L373 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L376-L396 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L399-L415

Vulnerability details

function totalSupplyAtEpoch(uint256 _epoch) external view returns (uint256 supply) {
    uint256 epochStart = uint256(epochs[_epoch].date).div(rewardsDuration).mul(
        rewardsDuration
    );
    uint256 cutoffEpoch = epochStart.sub(lockDuration);

    //traverse inversely to make more current queries more gas efficient
    for (uint256 i = _epoch; i + 1 != 0; i--) {
        Epoch storage e = epochs[i];
        if (uint256(e.date) <= cutoffEpoch) {
            break;
        }
        supply = supply.add(epochs[i].supply);
    }

    return supply;
}

In VE3DLocker.sol, there are multiple instances in which an inversely traverse for loop is used "to make more current queries more gas efficient".

For example:

• totalSupplyAtEpoch()
• balanceAtEpochOf()
• pendingLockAtEpochOf()
• totalSupply()

The implementation of the inversely traverse for loop is inherited from Convex's original version: https://github.com/convex-eth/platform/blob/main/contracts/contracts/CvxLockerV2.sol#L333-L334

However, Convex's locker contract is using Solidity 0.6.12, in which the arithmetic operations will overflow/underflow without revert.

As the solidity version used in the current implementation of VE3DLocker.sol is 0.8.7, and there are some breaking changes in Solidity v0.8.0, including:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.7/080-breaking-changes.html#silent-changes-of-the-semantics

Which makes the current implementation of inversely traverse for loops always reverts.

More specifically:

1. for (uint i = locks.length - 1; i + 1 != 0; i--) { will revert when locks.length == 0 at locks.length - 1 due to underflow;
2. for (uint256 i = _epoch; i + 1 != 0; i--) { will loop until i == 0 and reverts at i-- due to underflow.

As a result, all these functions will be malfunctioning and all the internal and external usage of these function will always revert.

Recommendation

Change VE3DLocker.sol#L315 to:

for (uint256 i = locks.length; i > 0; i--) {
    uint256 lockEpoch = uint256(locks[i - 1].unlockTime).sub(lockDuration);
    //lock epoch must be less or equal to the epoch we're basing from.
    if (lockEpoch <= epochTime) {
        if (lockEpoch > cutoffEpoch) {
            amount = amount.add(locks[i - 1].amount);

Change VE3DLocker.sol#L360 to:

for (uint256 i = locks.length; i > 0; i--) {
    uint256 lockEpoch = uint256(locks[i - 1].unlockTime).sub(lockDuration);

    //return the next epoch balance
    if (lockEpoch == nextEpoch) {
        return locks[i - 1].amount;
    } else if (lockEpoch < nextEpoch) {
        //no need to check anymore
        break;
    }

Change VE3DLocker.sol#L387 to:

for (uint256 i = epochindex; i > 0; i--) {
    Epoch storage e = epochs[i - 1];

Change VE3DLocker.sol#L406 to:

for (uint256 i = _epoch + 1; i > 0; i--) {
    Epoch storage e = epochs[i - 1];
    if (uint256(e.date) <= cutoffEpoch) {
        break;
    }
    supply = supply.add(e.supply);
}

[N] Critical changes should use two-step procedure

It's a best practice to use a two step process for changes of critical settings like setOwner().

Lack of two-step procedure for critical operations leaves them error-prone.

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L123-L127

    function setOwner(address _owner) external {
        require(msg.sender == owner, "!auth");
        owner = _owner;
        emit OwnerUpdated(_owner);
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L129-L134

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

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L135-L140

    function setPoolManager(address _poolM) external {
        require(msg.sender == poolManager, "!auth");
        poolManager = _poolM;
        emit PoolManagerUpdated(_poolM);
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L162-L166

    function setArbitrator(address _arb) external {
        require(msg.sender == owner, "!auth");
        rewardArbitrator = _arb;
        emit ArbitratorUpdated(_arb);
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L168-L172

    function setVoteDelegate(address _voteDelegate) external {
        require(msg.sender == voteDelegate, "!auth");
        voteDelegate = _voteDelegate;
        emit VoteDelegateUpdated(_voteDelegate);
    }

[L] Critical operations should emit events

Across the contracts, there are certain critical operations that change critical values that affect the users of the protocol.

It's a best practice for these setter functions to emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Instances include:

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L62-L65

    function setOwner(address _owner) external {
        require(msg.sender == owner, "!auth");
        owner = _owner;
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L67-L75

    function setOperator(address _operator) external {
        require(msg.sender == owner, "!auth");
        require(
            operator == address(0) || IDeposit(operator).isShutdown() == true,
            "needs shutdown"
        );

        operator = _operator;
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L77-L81

    function setDepositor(address _depositor) external {
        require(msg.sender == owner, "!auth");

        depositor = _depositor;
    }

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L83-L89

    function setStashAccess(address _stash, bool _status) external returns (bool) {
        require(msg.sender == operator, "!auth");
        if (_stash != address(0)) {
            stashPool[_stash] = _status;
        }
        return true;
    }

...

[L] Lack of input validation

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L582-L593

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

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L193-L197

function setFeeInfo(uint256 _lockFeesIncentive, uint256 _stakerLockFeesIncentive) external {
    require(msg.sender == feeManager, "!auth");

    lockFeesIncentive = _lockFeesIncentive;
    stakerLockFeesIncentive = _stakerLockFeesIncentive;

lockFeesIncentive and stakerLockFeesIncentive are used as they are in BPS. However, there is no check to ensure that the value does not exceed 10000.

[L] safeApprove has been deprecated

safeApprove has been deprecated in favor of safeIncreaseAllowance and safeDecreaseAllowance.

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L161-L162

    //stake for msg.sender
    IERC20(minter).safeApprove(_stakeAddress, _amount);

Can be changed to:

    IERC20(minter).safeIncreaseAllowance(_stakeAddress, _amount);

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S-1] Avoid unnecessary arithmetic operations can save gas

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L400-L402

uint256 epochStart = uint256(epochs[_epoch].date).div(rewardsDuration).mul(
    rewardsDuration
);

epochs[_epoch].date is a multiples of rewardsDuration, epochs[_epoch].date / rewardsDuration * rewardsDuration = epochs[_epoch].date

Recommendation

uint256 epochStart = uint256(epochs[_epoch].date);

[S-2] findEpochId() Implementation can be simpler and save some gas

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L418-L440

function findEpochId(uint256 _time) public view returns (uint256 epoch) {
    uint256 max = epochs.length - 1;
    uint256 min = 0;

    //convert to start point
    _time = _time.div(rewardsDuration).mul(rewardsDuration);

    for (uint256 i = 0; i < 128; i++) {
        if (min >= max) break;

        uint256 mid = (min + max + 1) / 2;
        uint256 midEpochBlock = epochs[mid].date;
        if (midEpochBlock == _time) {
            //found
            return mid;
        } else if (midEpochBlock < _time) {
            min = mid;
        } else {
            max = mid - 1;
        }
    }
    return min;
}

Recommendation

Change to:

function findEpochId(uint256 _time) public view returns (uint256 epoch) {
    return _time.sub(epochs[0].date).div(rewardsDuration);
}

[S-3] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• BaseRewardPool.sol

  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L176

  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L218

  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L245

  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L282

• PoolKeeper.sol#performUpkeepMultiplePools()

  https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L218

[S-4] Using immutable variable can save gas

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L55

    IERC20 public rewardToken;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L56

    IERC20 public stakingToken;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L62

    address public operator;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L63

    address public rewardManager;

Considering these variables will never change, changing to immutable variable instead of storage variable can save gas.

[M-5] Setting uint256 variables to 0 is redundant

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L66

    uint256 public periodFinish = 0;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L67

    uint256 public rewardRate = 0;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L70

    uint256 public queuedRewards = 0;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L71

    uint256 public currentRewards = 0;

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L72

    uint256 public historicalRewards = 0;

...

Setting uint256 variables to 0 is redundant as they default to 0.

[M-6] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in a loop.

For example:

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L176

        for (uint256 i = 0; i < extraRewards.length; i++) 

[M-7] Setting bool variables to false is redundant

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L106

    bool public isShutdown = false;

Setting bool variables to false is redundant as they default to false.

[M-8] Unused events

Unused events increase contract size and gas usage at deployment.

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L102

    event Voted(uint256 indexed voteId, address indexed votingAddress, bool support);

Voted is unused.

[S-9] Only using SafeMath when necessary can save gas

For the arithmetic operations that will never over/underflow, using SafeMath will cost more gas.

For example:

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L329-L332

        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(duration);
        } else {
            uint256 remaining = periodFinish.sub(block.timestamp);

periodFinish - block.timestamp will never underflow.

Recommendation

Change to:

        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(duration);
        } else {
            uint256 remaining = periodFinish - block.timestamp;