veToken Finance contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L134-L139 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L214-L216

Vulnerability details

Impact

Whenever a user stakes or withdraws funds through the VE3DRewardPool contract, the contract loops over the array. It calls the stake()/withdraw() function for each of the addresses. If the size of the array is too big, the transaction might reach the block gas limit. The contract would be unusable until the array is cleaned up through clearExtraRewards().

SWC-128

Proof of Concept

1. rewardManager calls addExtraReward() multiple times. At some point the array is too big.
2. Calls to stake(), stakeFor(), and withdraw() by users will fail

Tools Used

none

Recommended Mitigation Steps

Add a cap to the number of elements in the array. Same as in BaseRewardPool

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L134-L139 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L214-L216 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L121

Vulnerability details

Impact

When the same address is included twice it might cause issues depending on the contract. I checked the current convex contract to see which addresses were added to the extraRewards array. For cvxCRV Rewards there's only 0x7091dbb7fcbA54569eF1387Ac89Eb2a5C9F6d2EA which is the VirtualBalanceRewardPool contract.

Proof of Concept

1. rewardManager adds the same address twice through addExtraReward()
2. VE3DRewardPool calls stake() twice with the same amount:

    function stake(uint256 _amount) public updateReward(msg.sender) {
        require(_amount > 0, "RewardPool : Cannot stake 0");

        //also stake to linked rewards
        uint256 length = extraRewards.length;
        for (uint256 i = 0; i < length; i++) {
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
        }

        //add supply
        _totalSupply = _totalSupply.add(_amount);
        //add to sender balance sheet
        _balances[msg.sender] = _balances[msg.sender].add(_amount);
        //take tokens from sender
        stakingToken.safeTransferFrom(msg.sender, address(this), _amount);

        emit Staked(msg.sender, _amount);
    }

Tools Used

none

Recommended Mitigation Steps

Prevent the same addresses from being added multiple times to the extraRewards array.

Gas Report

G-01: cache array length in for loops

When looping over an array the condition of the loop is checked for every iteration. If you read the length of a storage variable multiple times it can get quite expensive. Instead, cache the length of memory and check that.

// bad
for (uint i; i < arr.length; i++) {

}

// good
uint length = arr.length;
for (uint i; i < length; i++) {

}

There are quite a number of instances where this can be used. Just search for "for (" and you'll find them.

G-02: cache storage variables that are read multiple times within a function

Whenever you read the same storage variable twice, you should cache it in memory. It will save you a SLOAD.

An example would be platformFee in _earmarkRewards() L526-L528