Rounding may lead to the reversal of certain transactions.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L81 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L114-L118 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L243

Vulnerability details

Impact

Occasionally, the claiming of rewards may be reversed due to rounding.

Proof of Concept

For the sake of this discussion, let's assume that the price of DAI is equal to that of ETH.

1. Alice initiates a deposit of (10100, 10100) into the WETH/DAI pool.
2. Add 20,000 SALT rewards for this pool. This is solely for testing purposes. Actual rewards are emitted from the RewardsEmitter. The result is

   Shares for Alice             ===>    20200
   Total Rewards                ===>    20000
   Rewards for Alice            ===>    20000

3. Bob deposits the same liquidity into the WETH/DAI pool. He cannot receive any rewards, but his virtual rewards have increased.

   function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal {
   uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );   // <== equal to 20000
   user.virtualRewards += uint128(virtualRewardsToAdd);  // <== 20000
   totalRewards[poolID] += uint128(virtualRewardsToAdd); // <== 40000
   }

   Log is

   Shares for Bob               ===>    20200
   Total Rewards                ===>    40000
   Virtual Rewards for Bob      ===>    20000

4. Charlie deposits (19800, 19800) into the WETH/DAI pool.

   Shares for Charlie           ===>    39600
   Total Rewards                ===>    79208
   Virtual Rewards for Charlie  ===>    39208

5. Bob withdraws 20,000 liquidity. In this case, Bob can receive 1 wei due to rounding.

   function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal {
   uint256 rewardsForAmount = ( totalRewards[poolID] * decreaseShareAmount ) / totalShares[poolID];  // <== 79208 * 20000 / (2 * 20200 + 39600) = 19802
   
   uint256 virtualRewardsToRemove = (user.virtualRewards * decreaseShareAmount) / user.userShare;   // <== 20000 * 20000 / 20200 = 19801
   if ( virtualRewardsToRemove < rewardsForAmount )
        claimableRewards = rewardsForAmount - virtualRewardsToRemove;
   }

   SALT Balance for Bob Before    ===>     0
   SALT Balance for Bob After     ===>     1

6. Alice aims to claim her reward, but the transaction will revert due to an exact lack of 1 wei in the balance.

   function userRewardForPool( address wallet, bytes32 poolID ) public view returns (uint256) {
   
   uint256 rewardsShare = ( totalRewards[poolID] * user.userShare ) / totalShares[poolID];  // <== 59406 * 20200 / (40400 + 39600 - 20000) = 20000
   }

   Claimable Rewards for Alice  ===>    20000
   Available SALT Balance       ===>    19999

The PoC for this is as below:

function testRounding() public {
    address charlie = address(0x3333);

    vm.startPrank(DEPLOYER);
    weth.transfer(alice, 10100);
    dai.transfer(alice, 10100);

    weth.transfer(bob, 10100);
    dai.transfer(bob, 10100);

    weth.transfer(charlie, 19800);
    dai.transfer(charlie, 19800);

    bytes32 poolID = PoolUtils._poolID(weth, dai);

    vm.startPrank(alice);
    weth.approve(address(collateralAndLiquidity), 10100);
    dai.approve(address(collateralAndLiquidity), 10100);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, dai, 10100, 10100, 0, block.timestamp, false);

    AddedReward[] memory addedRewards = new AddedReward[](1);
    addedRewards[0] = AddedReward(poolID, 20000);
    salt.approve(address(collateralAndLiquidity), 20000);
    collateralAndLiquidity.addSALTRewards(addedRewards);

    bytes32[] memory poolIDs = new bytes32[](1);
    poolIDs[0] = poolID;
    uint256[] memory totalRewards = new uint256[](1);
    totalRewards = collateralAndLiquidity.totalRewardsForPools(poolIDs);

    console.log('Shares for Alice             ===>   ', collateralAndLiquidity.userShareForPool(alice, poolID));
    console.log('Total Rewards                ===>   ', totalRewards[0]);
    console.log('Rewards for Alice            ===>   ', collateralAndLiquidity.userRewardForPool(alice, poolID));

    vm.startPrank(bob);
    weth.approve(address(collateralAndLiquidity), 10100);
    dai.approve(address(collateralAndLiquidity), 10100);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, dai, 10100, 10100, 0, block.timestamp, false);

    console.log('');
    console.log('Shares for Bob               ===>   ', collateralAndLiquidity.userShareForPool(bob, poolID));

    totalRewards = collateralAndLiquidity.totalRewardsForPools(poolIDs);

    console.log('Total Rewards                ===>   ', totalRewards[0]);
    console.log('Virtual Rewards for Bob      ===>   ', collateralAndLiquidity.userVirtualRewardsForPool(bob, poolID));

    vm.startPrank(charlie);
    weth.approve(address(collateralAndLiquidity), 19800);
    dai.approve(address(collateralAndLiquidity), 19800);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(weth, dai, 19800, 19800, 0, block.timestamp, false);

    console.log('');
    console.log('Shares for Charlie           ===>   ', collateralAndLiquidity.userShareForPool(charlie, poolID));

    totalRewards = collateralAndLiquidity.totalRewardsForPools(poolIDs);
    console.log('Total Rewards                ===>   ', totalRewards[0]);
    console.log('Virtual Rewards for Charlie  ===>   ', collateralAndLiquidity.userVirtualRewardsForPool(charlie, poolID));

    vm.startPrank(bob);
    vm.warp(block.timestamp + 7 days);
    uint256 saltBalanceBefore = salt.balanceOf(bob);
    collateralAndLiquidity.withdrawLiquidityAndClaim(weth, dai, 20000, 0, 0, block.timestamp);
    uint256 saltBalanceAfter = salt.balanceOf(bob);

    console.log('');
    console.log('SALT Balance for Bob Before    ===>    ', saltBalanceBefore);
    console.log('SALT Balance for Bob After     ===>    ', saltBalanceAfter);

    vm.startPrank(alice);

    console.log('');
    console.log('Claimable Rewards for Alice  ===>   ', collateralAndLiquidity.userRewardForPool(alice, poolID));
    console.log('Available SALT Balance       ===>   ', salt.balanceOf(address(collateralAndLiquidity)));

    vm.expectRevert("ERC20: transfer amount exceeds balance");
    collateralAndLiquidity.claimAllRewards(poolIDs);
}

Tools Used

Recommended Mitigation Steps

If the rewards exceed the balance, we can send the entire available balance.

Assessed type

Token-Transfer

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

Final claimAllRewards is not reverting as mentioned.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

Picodes removed the grade

[c4-judge]

Picodes marked the issue as duplicate of #1021

[c4-judge]

Picodes marked the issue as satisfactory