Velodrome Finance contest - horsefacts's results

Judge has assessed an item in Issue #164 as Medium risk. The relevant finding follows:

Low

Griefing/denial of service in notifyRewardAmount

Both Bribe#notifyRewardAmount and Gauge#notifyRewardAmount are unrestricted external functions that add the provided token as an approved reward if it does not already exist. A malicious caller can grief rewards functionality by calling these functions to add the maximum number of reward tokens on a new gauge/bribe contract, or set malicious tokens that revert or waste gas as rewards.

The team does have the ability to retroactively swap out misbehaving or malicious tokens, so the impact of this attack is limited, but they would have to detect and prevent potential griefing attacks for every new gauge/bribe.

Bribe#notifyRewardAmount

  function notifyRewardAmount(address token, uint amount) external lock {
      ...

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

      ...
  }

Gauge#notifyRewardAmount

    function notifyRewardAmount(address token, uint amount) external lock {
        ...

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

        ...
    }

Test cases (added to Bribes.t.sol):

    function testNotifyRewardAmountGaugeDoS() public {
        // Three rewards tokens already exist from test setup
        for (uint256 i; i < 13; ++i) {
          // Create a fake token
          MockERC20 token = new MockERC20("DoS", "DOS", 18);
          // Mint a sufficient amount to call notifyRewardAmount
          // on the Gauge contract
          token.mint(address(this), 10_000 ether);
          token.approve(address(gauge), 10_000 ether);
          // Call notifyRewardAmount and deposit
          gauge.notifyRewardAmount(address(token), 10_000 ether);
        }
        // Attempt to add a new token
        MockERC20 newToken = new MockERC20("Legit", "LEGIT", 18);
        newToken.mint(address(this), 1);
        newToken.approve(address(gauge), 1);
        vm.prank(address(bribe));
        vm.expectRevert("too many rewards tokens");
        gauge.addBribeRewardToken(address(newToken));
    }

    function testNotifyRewardAmountBribeDoS() public {
        // Three rewards tokens already exist from test setup
        for (uint256 i; i < 13; ++i) {
          // Create a fake token
          MockERC20 token = new MockERC20("DoS", "DOS", 18);
          token.mint(address(this), 1);
          token.approve(address(bribe), 1);
          // Call notifyRewardAmount and deposit 1 wei
          bribe.notifyRewardAmount(address(token), 1);
        }
        // Attempt to add a new token
        MockERC20 newToken = new MockERC20("Legit", "LEGIT", 18);
        newToken.mint(address(this), 1);
        newToken.approve(address(bribe), 1);
        vm.prank(address(gauge));
        vm.expectRevert("too many rewards tokens");
        bribe.addRewardToken(address(newToken));
    }

Recommendation: allow only approved tokens as rewards.

QA

Restrict function mutability

Mutability for Bribe#getEpochStart can be restricted to pure:

Bribe#getEpochStart

  function getEpochStart(uint timestamp) public view returns (uint) {
    uint bribeStart = timestamp - (timestamp % (7 days)) + BRIBE_LAG;
    uint bribeEnd = bribeStart + DURATION - COOLDOWN;
    return timestamp < bribeEnd ? bribeStart : bribeStart + 7 days;
  }

Use address.code.size

VotingEscrow#_isContract can use account.code.size to access codesize for an address rather than inline assembly in Solidiy 0.8.13

VotingEscrow#_isContract

    function _isContract(address account) internal view returns (bool) {
        // This method relies on extcodesize, which returns 0 for contracts in
        // construction, since the code is only stored at the end of the
        // constructor execution.
        uint size;
        assembly {
            size := extcodesize(account)
        }
        return size > 0;
    }

Prefer require to assert

VotingEscrow#withdraw uses an assert statement to check the result of an external ERC20 transfer. It's a best practice to use assert only for internal errors and invariants. Consider using require instead.

VotingEscrow#withdraw

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

Extract safe transfer library

The _safeTransfer and _safeTransferFrom functions are duplicated throughout the codebase. Consider extracting a shared library to reuse this code.

Duplicated code appears in Bribe.sol, Gauge.sol, Pair.sol, PairFees.sol, Router.sol, and Voter.sol.

Open TODOS

There are a number of open TODOs in comments throughout the codebase. Review and remove these if they are resolved.

• VotingEscrow#L314
• VotingEscrow#L465
• VotingEscrow#L524
• VelodromeLibrary#L9
• `Minter#L11

Gas

Optimize loop in Gauge#getReward

The rewards claiming loop in Gauge#getReward can be optimized with a few loop optimization tricks: caching loop length, using unchecked increments, and caching the intermediate token value. Since this will be a frequently called function, it's probably worth the extra optimization:

Gauge#getReward

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

Recommendation:

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

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

            emit ClaimRewards(msg.sender, token, _reward);
            unchecked { ++i; }
        }