Velodrome Finance contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L526

Vulnerability details

Users who are approved, but do not own a particular NFT, are supposed to be eligible to call merge and withdraw from the NFT.

Currently _burn(), used by merge() and withdraw() to remove the NFT from the system, will revert unless the sender is the owner of NFT as the function tries to update the accounting for the sender, not the owner.

Setting the severity to medium as the impact is merge() and withdraw() permanent unavailability for any approved sender, who isn't the owner of the involved NFT.

Proof of Concept

_removeTokenFrom() requires _from to be the NFT owner as it removes _tokenId from the _from account:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L504-L515

    /// @dev Remove a NFT from a given address
    ///      Throws if `_from` is not the current owner.
    function _removeTokenFrom(address _from, uint _tokenId) internal {
        // Throws if `_from` is not the current owner
        assert(idToOwner[_tokenId] == _from);
        // Change the owner
        idToOwner[_tokenId] = address(0);
        // Update owner token index tracking
        _removeTokenFromOwnerList(_from, _tokenId);
        // Change count tracking
        ownerToNFTokenCount[_from] -= 1;
    }

_burn() allows _tokenId to approved or owner, but calls _removeTokenFrom() with msg.sender as _from:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L517-L528

    function _burn(uint _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");

        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // TODO add delegates
        // Remove token
        _removeTokenFrom(msg.sender, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
    }

This way if _burn() is called by an approved account who isn't an owner, it will revert on _removeTokenFrom()'s assert(idToOwner[_tokenId] == _from) check.

Now burn() is used by merge():

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L1084-L1097

    function merge(uint _from, uint _to) external {
        require(attachments[_from] == 0 && !voted[_from], "attached");
        require(_from != _to);
        require(_isApprovedOrOwner(msg.sender, _from));
        require(_isApprovedOrOwner(msg.sender, _to));

        LockedBalance memory _locked0 = locked[_from];
        LockedBalance memory _locked1 = locked[_to];
        uint value0 = uint(int256(_locked0.amount));
        uint end = _locked0.end >= _locked1.end ? _locked0.end : _locked1.end;

        locked[_from] = LockedBalance(0, 0);
        _checkpoint(_from, _locked0, LockedBalance(0, 0));
        _burn(_from);

And withdraw():

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L842-L864

    /// @notice Withdraw all tokens for `_tokenId`
    /// @dev Only possible if the lock has expired
    function withdraw(uint _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");

        LockedBalance memory _locked = locked[_tokenId];
        require(block.timestamp >= _locked.end, "The lock didn't expire");
        uint value = uint(int256(_locked.amount));

        locked[_tokenId] = LockedBalance(0,0);
        uint supply_before = supply;
        supply = supply_before - value;

        // old_locked can have either expired <= timestamp or zero end
        // _locked has only 0 end
        // Both can have >= 0 amount
        _checkpoint(_tokenId, _locked, LockedBalance(0,0));

        assert(IERC20(token).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

Recommended Mitigation Steps

Consider changing _removeTokenFrom() argument to be the owner:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L517-L528

    function _burn(uint _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");

        address owner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // TODO add delegates
        // Remove token
-       _removeTokenFrom(msg.sender, _tokenId);
+       _removeTokenFrom(owner, _tokenId);
        emit Transfer(owner, address(0), _tokenId);
    }

Lines of code

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

Vulnerability details

Any user balance affecting action, i.e. deposit, withdraw/withdrawToken or getReward, calls _writeCheckpoint to update the balance records used for the earned reward estimation. The issue is that _writeCheckpoint always sets false to voted flag for the each new checkpoint due to wrong index used in the mapping access, while only voted periods are eligible for accruing the rewards.

This way any balance changing action of a voted user will lead to stopping of the rewards accrual for the user, until next vote will be cast. I.e. any action that has no relation to voting and should have only balance change as the reward accruing process impact, in fact removes any future rewards from the user until the next vote.

Setting the severity to be high as the impact here violates system logic and means next periods accrued rewards loss for a user.

Proof of Concept

_writeCheckpoint adds a new checkpoint if block.timestamp is not found in the last checkpoint:

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

    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;
        }
    }

However, instead of moving vote status from the previous checkpoint it records false to prevVoteStatus all the time as last status is checkpoints[account][_nCheckPoints-1].voted, while checkpoints[account][_nCheckPoints] isn't created yet and is empty:

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

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

Notice that checkpoints is a mapping and no range check violation happens:

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

    /// @notice A record of balance checkpoints for each account, by index
    mapping (address => mapping (uint => Checkpoint)) public checkpoints;

This will effectively lead to rewards removal on any user action, as earned() used in rewards estimation counts only voted periods:

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

        if (_endIndex > 0) {
            for (uint i = _startIndex; i < _endIndex; i++) {
                Checkpoint memory cp0 = checkpoints[account][i];
                Checkpoint memory cp1 = checkpoints[account][i+1];
                (uint _rewardPerTokenStored0,) = getPriorRewardPerToken(token, cp0.timestamp);
                (uint _rewardPerTokenStored1,) = getPriorRewardPerToken(token, cp1.timestamp);
                if (cp0.voted) {
                    reward += cp0.balanceOf * (_rewardPerTokenStored1 - _rewardPerTokenStored0) / PRECISION;
                }
            }
        }

        Checkpoint memory cp = checkpoints[account][_endIndex];
        uint lastCpWeeksVoteEnd = cp.timestamp - (cp.timestamp % (7 days)) + BRIBE_LAG + DURATION;
        if (block.timestamp > lastCpWeeksVoteEnd) {
            (uint _rewardPerTokenStored,) = getPriorRewardPerToken(token, cp.timestamp);
            if (cp.voted) {
                reward += cp.balanceOf * (rewardPerToken(token) - Math.max(_rewardPerTokenStored, userRewardPerTokenStored[token][account])) / PRECISION;
            }
        }

I.e. if a user has voted, then performed any of the operations that call _writeCheckpoint update: deposit, withdraw/withdrawToken or getReward, then this user will not have any rewards for the period between this operation and the next vote as all checkpoints that were created by _writeCheckpoint will have voted == false.

Recommended Mitigation Steps

Update the index to be _nCheckPoints-1:

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

-	bool prevVoteStatus = (_nCheckPoints > 0) ? checkpoints[account][_nCheckPoints].voted : false;
+	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#L30-L33

Vulnerability details

If Bribe and Gauge constructors are run not in the same transaction, the griefing attack is possible. A malicious user can run setGauge after Bribe, but before Gauge constructor, making Bribe contract unusable. The fix here is Bribe redeployment.

Setting severity to be medium as that is temporary system breaking impact.

Proof of Concept

setGauge can be run by anyone, but only once with a meaningful gauge:

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

  function setGauge(address _gauge) external {
    require(gauge == address(0), "gauge already set");
    gauge = _gauge;
  }

Now it is called in Gauge constructor:

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

    constructor(address _stake, address _bribe, address  __ve, address _voter, bool _isForPair) {
        stake = _stake;
        bribe = _bribe;
        _ve = __ve;
        voter = _voter;

        factory = msg.sender;

        IBribe(bribe).setGauge(address(this));

This way it will not be called before Gauge constructor, but if it is not atomic with Bribe constructor, an attacker can call in-between.

Recommended Mitigation Steps

Consider either running Bribe and then Gauge constructors atomically, or introducing an owner role in Bribe constructor and onlyOwner access control in setGauge, setting it manually.

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L590-L595 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L617-L621

Vulnerability details

Griefing attack is possible as Gauge's and Bribe's notifyRewardAmount() allows for adding the reward token by anyone up to MAX_REWARD_TOKENS. This will disable the possibility of the further addition of reward tokens and clutter the system as reward token array cycle is used in Gauge's deliverBribes(), deposit() and withdraw() (via _updateRewardForAllTokens()).

Neither Gauge, nor Bribe have the ability to remove reward tokens, so the attack will have permanent impact on the deployed contracts. Making this a combined issue as Gauge's notifyRewardAmount() calls Bribe's addRewardToken(), and Bribe's notifyRewardAmount() calls Gauge's addBribeRewardToken() with the similar functionality, i.e. attack on one of them is equivalent to attack on another.

The inability to add reward tokens is a violation of system's logic and substantial slowing down of user-facing deposit() and withdraw() affects all the users and raises cumulative gas costs of system's usage for them, so marking severity here as high.

Proof of Concept

Gauge's notifyRewardAmount() do not have access controls:

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

    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");
        }

notifyRewardAmount() also adds reward token if it is not registered yet:

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

        if (!isReward[token]) {
            isReward[token] = true;
            rewards.push(token);
            IBribe(bribe).addRewardToken(token);
        }

There is no lower limit on a provided amount for any new token, e.g. 1 wei will suffice:

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

        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;

This way any caller can add any token as a reward one. This makes a griefing attack possible, when a malicious user calls notifyRewardAmount(token, amount) with a tiny amount and a different token each time, filling up the reward token array, rewards.

anyone can fill up the reward query up the maximum, preventing from any further addition of reward tokens and slowing down the system.

Similarly, Bribe's notifyRewardAmount have no access controls and do not require a reward token to be registered, only checks the maximum:

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

  function notifyRewardAmount(address token, uint amount) external lock {
      require(amount > 0);
      if (!isReward[token]) {
        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
      }

And adds it to the rewards of both contracts if the token is previously unknown:

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

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

Gauge's deliverBribes(), deposit() and withdraw() functions loop over rewards array, so the attack will substantially slow down and raise gas costs for the system, as a loop over usual 1-3 reward tokens will be extended to 16:

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

uint internal constant MAX_REWARD_TOKENS = 16;

There seems to be no way to remove a reward token in Gauge and Bribe, so the impact is permanent.

Recommended Mitigation Steps

Consider making Gauge's and Bribe's notifyRewardAmount() require isReward[token], and introducing permissioned addRewardToken() to Gauge so only registered actors can run it.

Alternatively, if there is a desire to keep the system permissionless, consider introducing the oracle and requiring substantial enough minimum predefined value to be locked on new token addition, so a tiny amount in the attack will not suffice. Also, as an additional measure, consider reducing the MAX_REWARD_TOKENS, as lower values, like 5 or 7, will suffice for the most practical purposes.

Lines of code

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

Vulnerability details

_updateRewardForAllTokens is called in withdraw() only, while both withdraw() and withdrawToken() are public and can be called directly. Reward update is needed on user's balance change, its absence leads to reward data rewriting and rewards losing impact for a user.

Per discussion with project withdrawToken() will not be exposed in UI, so setting the severity to be medium as user's direct usage of the vulnerable function is required here, while the impact is losing of a part of accumulated rewards.

Proof of Concept

withdraw() is a wrapper for withdrawToken(), which changes user's balance. withdrawToken() can be called directly, and in this case there will be no rewards update:

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

    function withdraw(uint amount) public {
        _updateRewardForAllTokens();

        uint tokenId = 0;
        if (amount == balanceOf[msg.sender]) {
            tokenId = tokenIds[msg.sender];
        }
        withdrawToken(amount, tokenId);
    }

    function withdrawToken(uint amount, uint tokenId) public lock {
        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

This way if a user call withdrawToken(), the associated rewards can be lost due to incomplete reward balance accounting.

References

A showcase of reward update issue:

https://github.com/belbix/solidly/issues/1

It's replicated live with users losing accumulated rewards:

https://github.com/solidlyexchange/solidly/issues/55

Recommended Mitigation Steps

Consider moving rewards update to withdrawToken(), since it is called by withdraw() anyway:

    function withdraw(uint amount) public {
-       _updateRewardForAllTokens();

        uint tokenId = 0;
        if (amount == balanceOf[msg.sender]) {
            tokenId = tokenIds[msg.sender];
        }
        withdrawToken(amount, tokenId);
    }

    function withdrawToken(uint amount, uint tokenId) public lock {
+    	_updateRewardForAllTokens();

        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

Or, if the intent is to keep withdrawToken() as is, consider making it private, so no direct usage be possible:

-    function withdrawToken(uint amount, uint tokenId) public lock {
+    function withdrawToken(uint amount, uint tokenId) private lock {

        totalSupply -= amount;
        balanceOf[msg.sender] -= amount;
        _safeTransfer(stake, msg.sender, amount);

1. Minter comment states token abstraction support, which isn't now in place (low)

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Minter.sol#L10-L11

// codifies the minting rules as per ve(3,3), abstracted from the token to support any token that allows minting
// TODO: decide on whether to abstract from VELO or not. currently it's only somewhat abstracted (e.g. L38)

Recommended Mitigation Steps

Consider simplifying the comment to state that currently the system supports VELO token only as current Minter contract implementation aren’t universal, first of all it doesn’t allow tokens with decimals different from 18 (several functions break up on different decimals).

2. Test chain id is left in the code (non-critical)

Both FTM testnet and Kovan ids are left in release code:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/redeem/RedemptionReceiver.sol#L11-L24

contract RedemptionReceiver is ILayerZeroReceiver {
    IERC20 public immutable USDC;
    IVelo public immutable VELO;
    uint16 public immutable fantomChainId; // 12 for FTM, 10012 for FTM testnet
	...

    constructor(
    	...
    ) {
        require(_fantomChainId == 12 || _fantomChainId == 10012, "CHAIN_ID_NOT_FTM");

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/redeem/RedemptionSender.sol#L9-L21

contract RedemptionSender {
    address public immutable weve;
    uint16 public immutable optimismChainId; // 11 for OP, 10011 for OP Kovan
    ...

    constructor(
    	...
    ) {
        require(_optimismChainId == 11 || _optimismChainId == 10011, "CHAIN_ID_NOT_OP");

Recommended Mitigation Steps

Consider removing test chain ids from the production code to make it cleaner and reduce further development mistakes possibility.

3. Comment irrelevant (non-critical)

// Delete should be put several lines lower, here it is not really relevant:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L474-L480

    /// @dev Remove a NFT from an index mapping to a given address
    /// @param _from address of the sender
    /// @param _tokenId uint ID Of the token to be removed
    function _removeTokenFromOwnerList(address _from, uint _tokenId) internal {
        // Delete
        uint current_count = _balance(_from) - 1;
        uint current_index = tokenToOwnerIndex[_tokenId];

4. Comment misspelling (non-critical)

To be /// @dev Execute:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L295

/// @dev Exeute transfer of a NFT.

5. Open TODOs in comments (non-critical)

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VelodromeLibrary.sol#L9

IRouter internal immutable router; // TODO make modifiable?

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L314

// TODO delegates

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L465

// TODO add delegates

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L524

// TODO add delegates

Recommended Mitigation Steps

Consider removing TODO comments from the production code

6. Assert is used instead of require in a number of contracts (non-critical)

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.

Proof of Concept

assert is used instead of require across the system:

RewardsDistributor contract:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/RewardsDistributor.sol#L98

assert(msg.sender == depositor);

4 occurrences in Router contract, starting with:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Router.sol#L36

assert(msg.sender == address(weth)); // only accept ETH via fallback from the WETH contract

13 occurrences in VotingEscrow contract, starting with:

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/VotingEscrow.sol#L262

assert(_operator != msg.sender);

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases.