Velodrome Finance contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L302-L313

Vulnerability details

Impact

All the Voting values calculated by _writeCheckpoint() (when it's not first checkpoint) is going to set to False instead of account's last vote and because vote has been used in earned() and reward calculation so reward distribution is going to be wrong too.

Proof of Concept

This is _writeCheckpoint() code in Gauge:

    function _writeCheckpoint(address account, uint balance) internal {
        uint _timestamp = block.timestamp;
        uint _nCheckPoints = numCheckpoints[account];

        if (_nCheckPoints > 0 && checkpoints[account][_nCheckPoints - 1].timestamp == _timestamp) {
            checkpoints[account][_nCheckPoints - 1].balanceOf = balance;
        } else {
            bool prevVoteStatus = (_nCheckPoints > 0) ? checkpoints[account][_nCheckPoints].voted : false;
            checkpoints[account][_nCheckPoints] = Checkpoint(_timestamp, balance, prevVoteStatus);
            numCheckpoints[account] = _nCheckPoints + 1;
        }
    }

As you can see in the line bool prevVoteStatus = (_nCheckPoints > 0) ? checkpoints[account][_nCheckPoints].voted : false; it uses index _nCheckPoints to get the previous vote status but it should have used _nCheckPoints - 1 as index to get previous vote status becasue the last vote is in index _nCheckPoints - 1

Tools Used

VIM

Recommended Mitigation Steps

change the line to this:

            bool prevVoteStatus = (_nCheckPoints > 0) ? checkpoints[account][_nCheckPoints-1].voted : false;

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L92-L105 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L664-L676 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L41-L60 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L511-L520 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L603-L612

Vulnerability details

Impact

The actual amount that has been transferred can be different than requested amount in deflationary tokens and this is not been addressed in transferring logic in the code. This can cause wrong calculation and rewards distribution for users.

Proof of Concept

This is deposit() code in Gauge:

    function deposit(uint amount, uint tokenId) public lock {
        require(amount > 0);

        _updateRewardForAllTokens();

        _safeTransferFrom(stake, msg.sender, address(this), amount);
        totalSupply += amount;
        balanceOf[msg.sender] += amount;

        if (tokenId > 0) {
            require(IVotingEscrow(_ve).ownerOf(tokenId) == msg.sender);
            if (tokenIds[msg.sender] == 0) {
                tokenIds[msg.sender] = tokenId;
                IVoter(voter).attachTokenToGauge(tokenId, msg.sender);
            }
            require(tokenIds[msg.sender] == tokenId);
        } else {
            tokenId = tokenIds[msg.sender];
        }

        uint _derivedBalance = derivedBalances[msg.sender];
        derivedSupply -= _derivedBalance;
        _derivedBalance = derivedBalance(msg.sender);
        derivedBalances[msg.sender] = _derivedBalance;
        derivedSupply += _derivedBalance;

        _writeCheckpoint(msg.sender, _derivedBalance);
        _writeSupplyCheckpoint();

        IVoter(voter).emitDeposit(tokenId, msg.sender, amount);
        emit Deposit(msg.sender, tokenId, amount);
    }

As you can see it calls _safeTransferFrom(stake, msg.sender, address(this), amount); to get user tokens and then it adds amount to user balance and total supply: totalSupply += amount; balanceOf[msg.sender] += amount; but the actual transferred amount could be different in deflationary tokens and this can cause wrong reward distributions. The other instances of the bug are similar.

Tools Used

VIM

Recommended Mitigation Steps

check for actual transferred amount when transferring.

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L41-L60 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L590-L624

Vulnerability details

Impact

It's possible to call notifyRewardAmount() in Bribe or Gauge contract with malicious tokens and contract will add them to reward tokens list and then attacker can interrupt Gauge.deliverBribes() logic (by failing all contract transaction in that malicious token). because Gauge.deliverBribes() is looping through all tokens and transferring rewards and if one of them fails whole transaction will fail. As Gauge.deliverBribes() is called by Voter.distribute() and Voter.distribute() is called by Gauge.getReward() so functions: Voter.distribute() and Gauge.getReward() will be broke too and no one can call them for that Gauge.

Proof of Concept

This is notifyRewardAmount() code in Bribe:

  function notifyRewardAmount(address token, uint amount) external lock {
      require(amount > 0);
      if (!isReward[token]) {
        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
      }
      // bribes kick in at the start of next bribe period
      uint adjustedTstamp = getEpochStart(block.timestamp);
      uint epochRewards = tokenRewardsPerEpoch[token][adjustedTstamp];

      _safeTransferFrom(token, msg.sender, address(this), amount);
      tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;

      if (!isReward[token]) {
          isReward[token] = true;
          rewards.push(token);
          IGauge(gauge).addBribeRewardToken(token);
      }

      emit NotifyReward(msg.sender, token, adjustedTstamp, amount);
  }

As you can see it's callable by anyone and it will add the new tokens to rewards list in Bribe and Gauge contract. notifyRewardAmount() in Gauge is similar to Bribe's notifyRewardAmount(). This is deliverBribes() code in Gauge:

    function deliverBribes() external lock {
        require(msg.sender == voter);
        IBribe sb = IBribe(bribe);
        uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
        uint numRewards = sb.rewardsListLength();

        for (uint i = 0; i < numRewards; i++) {
            address token = sb.rewards(i);
            uint epochRewards = sb.deliverReward(token, bribeStart);
            if (epochRewards > 0) {
                _notifyBribeAmount(token, epochRewards, bribeStart);
            }
        }
    }

As you can see it loops through all rewards token list and calls Bribe.deliverReward() which then transfers the reward token. but if one of the tokens were malicious and revert the transaction then the whole transaction will fail and deliverBribes() logic will be blocked. This is distribute() code in Voter:

    function distribute(address _gauge) public lock {
        require(isAlive[_gauge]); // killed gauges cannot distribute
        uint dayCalc = block.timestamp % (7 days);
        require((dayCalc < BRIBE_LAG) || (dayCalc > (DURATION + BRIBE_LAG)), "cannot claim during votes period");
        IMinter(minter).update_period();
        _updateFor(_gauge);
        uint _claimable = claimable[_gauge];
        if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
            claimable[_gauge] = 0;
            IGauge(_gauge).notifyRewardAmount(base, _claimable);
            emit DistributeReward(msg.sender, _gauge, _claimable);
            // distribute bribes & fees too
            IGauge(_gauge).deliverBribes();
        }
    }

As you can see it calls Gauge.deliverBribes(). and this is getReward() code in Gauge:

    function getReward(address account, address[] memory tokens) external lock {
        require(msg.sender == account || msg.sender == voter);
        _unlocked = 1;
        IVoter(voter).distribute(address(this));
        _unlocked = 2;

        for (uint i = 0; i < tokens.length; i++) {
            (rewardPerTokenStored[tokens[i]], lastUpdateTime[tokens[i]]) = _updateRewardPerToken(tokens[i]);

            uint _reward = earned(tokens[i], account);
            lastEarn[tokens[i]][account] = block.timestamp;
            userRewardPerTokenStored[tokens[i]][account] = rewardPerTokenStored[tokens[i]];
            if (_reward > 0) _safeTransfer(tokens[i], account, _reward);

            emit ClaimRewards(msg.sender, tokens[i], _reward);
        }

        uint _derivedBalance = derivedBalances[account];
        derivedSupply -= _derivedBalance;
        _derivedBalance = derivedBalance(account);
        derivedBalances[account] = _derivedBalance;
        derivedSupply += _derivedBalance;

        _writeCheckpoint(account, derivedBalances[account]);
        _writeSupplyCheckpoint();
    }

which calls Voter.distribute(). so if Gauge.deliverBribes() fails then the logic of Voter.distribute() and Gauge.getReward() will fail too and users couldn't call them and attacker can cause DOS. Of course the team can swap those malicious reward tokens added by attacker with swapOutRewardToken() and swapOutBribeRewardToken() but for some time those logics will not work and attacker can cause DOS.

Tools Used

VIM

Recommended Mitigation Steps

set access levels for notifyRewardAmount() functions.

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Voter.sol#L315-L329

Vulnerability details

Impact

Voter.distribute() calls Gauge.deliverBribes() if claimable[_gauge] / DURATION > 0 was True and claimable[_gauge] shows base token rewards for gauge. Gauge.deliverBribes() calls Bribe.deliverReward() which transfers the rewards to Gauge. so for Bribe rewards to been transferred and distributed claimable[_gauge] / DURATION > 0 most be True and if there was some new rewards in Bribe and that condition is not True then those rewards will stuck in Bribe contract.

Proof of Concept

This is Voter.distribute() code:

    function distribute(address _gauge) public lock {
        require(isAlive[_gauge]); // killed gauges cannot distribute
        uint dayCalc = block.timestamp % (7 days);
        require((dayCalc < BRIBE_LAG) || (dayCalc > (DURATION + BRIBE_LAG)), "cannot claim during votes period");
        IMinter(minter).update_period();
        _updateFor(_gauge);
        uint _claimable = claimable[_gauge];
        if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
            claimable[_gauge] = 0;
            IGauge(_gauge).notifyRewardAmount(base, _claimable);
            emit DistributeReward(msg.sender, _gauge, _claimable);
            // distribute bribes & fees too
            IGauge(_gauge).deliverBribes();
        }
    }

As you can see IGauge(_gauge).deliverBribes(); is inside and if which checks _claimable / DURATION > 0 for that Gauge. Bribe is transferring rewards in Bribe.deliverReward() which is called by Gauge.deliverBribes() and it is called by Voter.distribute() if some base token claimable amount of that Gauge is bigger than DURATION, so if this condition isn't true, then rewards in Bribe for that epoch will stuck in Bribe. Voter.distribute() can be called multiple times in each time claimable[_gauge] will set to 0 so if Bribe rewards was after that call then those rewards will stuck in Bribe for sure.

Tools Used

VIM

Recommended Mitigation Steps

Move the line IGauge(_gauge).deliverBribes(); out of If in Voter.distribute()

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L83-L90

Vulnerability details

Impact

Function deliverReward() in Bribe contract won't set tokenRewardsPerEpoch[token][epochStart] to 0 after transferring rewards. Gauge.getReward() calls Voter.distribute() which calls Gauge.deliverBribes() which calls Bribe.deliverReward(). so if Gauge.getReward() or Voter.distribute() get called multiple times in same epoch then deliverReward() will transfer Bribe tokens multiple times because it doesn't set tokenRewardsPerEpoch[token][epochStart] to 0 after transferring.

Proof of Concept

This is deliverReward() code in Bribe:

  function deliverReward(address token, uint epochStart) external lock returns (uint) {
    require(msg.sender == gauge);
    uint rewardPerEpoch = tokenRewardsPerEpoch[token][epochStart];
    if (rewardPerEpoch > 0) {
      _safeTransfer(token, address(gauge), rewardPerEpoch);
    }
    return rewardPerEpoch;
  }

As you can see it doesn't set tokenRewardsPerEpoch[token][epochStart] value to 0, so if this function get called multiple times it will transfer epoch rewards multiple times (it will use other epoch's rewards tokens). function Gauge.deliverBribes() calls Bribe.deliverReward() and Gauge.deliverBribes() is called by Voter.distribute() if the condition claimable[_gauge] > DURATION is True. This is those functions codes:

    function deliverBribes() external lock {
        require(msg.sender == voter);
        IBribe sb = IBribe(bribe);
        uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
        uint numRewards = sb.rewardsListLength();

        for (uint i = 0; i < numRewards; i++) {
            address token = sb.rewards(i);
            uint epochRewards = sb.deliverReward(token, bribeStart);
            if (epochRewards > 0) {
                _notifyBribeAmount(token, epochRewards, bribeStart);
            }
        }
    }

    function distribute(address _gauge) public lock {
        require(isAlive[_gauge]); // killed gauges cannot distribute
        uint dayCalc = block.timestamp % (7 days);
        require((dayCalc < BRIBE_LAG) || (dayCalc > (DURATION + BRIBE_LAG)), "cannot claim during votes period");
        IMinter(minter).update_period();
        _updateFor(_gauge);
        uint _claimable = claimable[_gauge];
        if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
            claimable[_gauge] = 0;
            IGauge(_gauge).notifyRewardAmount(base, _claimable);
            emit DistributeReward(msg.sender, _gauge, _claimable);
            // distribute bribes & fees too
            IGauge(_gauge).deliverBribes();
        }
    }

also Gauge.getReward() calls Voter.getReward(). condition claimable[_gauge] > DURATION in Voter.distribute() can be true multiple time in one epoch (deliverBribes() would be called multiple times) because claimable[_gauge] is based on index and index increase by notifyRewardAmount() in Voter anytime.

Tools Used

VIM

Recommended Mitigation Steps

set tokenRewardsPerEpoch[token][epochStart] to 0 in deliverReward

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L648-L662 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L590-L624

Vulnerability details

Impact

In _notifyBribeAmount() and notifyRewardAmount() code calculates rewardRate[token] and to do that, first it transfers amount and then set rewardRate[token] = amount / DURATION so because of rounding error in division some of transferred rewards wouldn't be distributed among users and they will be locked in contract.

Proof of Concept

This is _notifyBribeAmount() and notifyRewardAmount() code in Gauge:

   function notifyRewardAmount(address token, uint amount) external lock {
        require(token != stake);
        require(amount > 0);
        if (!isReward[token]) {
            require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
        }
        // rewards accrue only during the bribe period
        uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
        uint adjustedTstamp = block.timestamp < bribeStart ? bribeStart : bribeStart + 7 days;
        if (rewardRate[token] == 0) _writeRewardPerTokenCheckpoint(token, 0, adjustedTstamp);
        (rewardPerTokenStored[token], lastUpdateTime[token]) = _updateRewardPerToken(token);
        _claimFees();

        if (block.timestamp >= periodFinish[token]) {
            _safeTransferFrom(token, msg.sender, address(this), amount);
            rewardRate[token] = amount / DURATION;
        } else {
            uint _remaining = periodFinish[token] - block.timestamp;
            uint _left = _remaining * rewardRate[token];
            require(amount > _left);
            _safeTransferFrom(token, msg.sender, address(this), amount);
            rewardRate[token] = (amount + _left) / DURATION;
        }
        require(rewardRate[token] > 0);
        uint balance = IERC20(token).balanceOf(address(this));
        require(rewardRate[token] <= balance / DURATION, "Provided reward too high");
        periodFinish[token] = adjustedTstamp + DURATION;
        if (!isReward[token]) {
            isReward[token] = true;
            rewards.push(token);
            IBribe(bribe).addRewardToken(token);
        }

        emit NotifyReward(msg.sender, token, amount);
    }

    function _notifyBribeAmount(address token, uint amount, uint epochStart) internal {
        if (block.timestamp >= periodFinish[token]) {
            rewardRate[token] = amount / DURATION;
        } else {
            uint _remaining = periodFinish[token] - block.timestamp;
            uint _left = _remaining * rewardRate[token];
            require(amount > _left);
            rewardRate[token] = (amount + _left) / DURATION;
        }

        lastUpdateTime[token] = epochStart;
        periodFinish[token] = epochStart + DURATION;

        emit NotifyReward(msg.sender, token, amount);
    }

As you can see to calculate rewardRate[token] the transferred amount is divided by DURATION so some of the rewards is not going to be distributed because of the rounding error here and code don't cumulative these extra rewards to use them again and rounding error happens in every call.

Tools Used

VIM

Recommended Mitigation Steps

send back extra rewards or keep track of extra rewards and cumulative them and then distribute them.