Claimed rewards are not accumulated, causing stealing of rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L63

Vulnerability details

Impact

Some StakingFundsVault stakers will get more rewards than expected while others will be locked out of claiming rewards due to insufficient funds.

Proof of Concept

When tracking rewards claimed by users, only the currently claimed amount is tracked, but it should accumulate all claimed rewards by a user-token pair (SyndicateRewardsProcessor.sol#L63):

uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
if (due > 0) {
    claimed[_user][_token] = due; // @audit must accrue claimed rewards

    totalClaimed += due;

    (bool success, ) = _recipient.call{value: due}("");
    require(success, "Failed to transfer");

    emit ETHDistributed(_user, _recipient, due);
}

This allows stakers to receive a higher reward, basically stealing it from other stakers:

// test/foundry/StakingFundsVault.t.sol
function testNonAccumulatingClaimedRewards_AUDIT() public {
    // Resetting the mocks, we need real action.
    MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 0);
    liquidStakingManager.setIsPartOfNetwork(blsPubKeyOne, false);

    // Aliasing accounts for better readability.
    address nodeRunner = accountOne;
    address alice = accountTwo;
    address bob = accountThree;
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    // Node runner registers a BLS key.
    registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFive);

    uint256[] memory amounts = new uint256[](1);
    amounts[0] = maxStakingAmountPerValidator / 2;

    // Alice and Bob stake 50/50, their reward share will be equal.
    depositETH(alice, maxStakingAmountPerValidator / 2, amounts, getBytesArrayFromBytes(blsPubKeyOne));
    depositETH(bob, maxStakingAmountPerValidator / 2, amounts, getBytesArrayFromBytes(blsPubKeyOne));

    // Someone elese deposits to the savETH vault of the first key.
    liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(blsPubKeyOne, 24 ether);

    // The first validator is registered and the derivatives are minted.
    assertEq(vault.totalShares(), 0);
    stakeAndMintDerivativesSingleKey(blsPubKeyOne);
    assertEq(vault.totalShares(), 4 ether);

    // Warping to pass the lastInteractedTimestamp checks.
    vm.warp(block.timestamp + 1 hours);

    assertEq(alice.balance, 0);
    assertEq(bob.balance, 0);

    LPToken lpTokenBLSPubKeyOne = vault.lpTokenForKnot(blsPubKeyOne);

    // Issuing first round of rewards.
    uint256 rewardsAmount = 1 ether;
    vm.deal(address(vault), rewardsAmount);
    assertEq(address(vault).balance, rewardsAmount);
    // Alice and Bob will get 50% of the reward each.
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount / 2);
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), rewardsAmount / 2);

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    assertEq(alice.balance, 0.5 ether);
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 0.5 ether);

    // Issuing second round of rewards.
    vm.deal(address(vault), address(vault).balance + rewardsAmount);
    // The vault is holding: this round's reward + the Bob's share from the previous round.
    assertEq(address(vault).balance, rewardsAmount + (rewardsAmount / 2));
    // Alice's share: 50% in the second round.
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount / 2);
    // Bob's share: 50% in the first round and 50% in the second one.
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), 2 * (rewardsAmount / 2));

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    assertEq(alice.balance, 1 ether);
    // The claimed amount is set to 0.5 ETH (this round's claimed reward) instead of 1 ETH.
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 0.5 ether);

    // Issuing third round of rewards.
    vm.deal(address(vault), address(vault).balance + rewardsAmount);
    // Current round + the Bob's two unclaimed rewards from the previous rounds.
    assertEq(address(vault).balance, rewardsAmount + 2 * (rewardsAmount / 2));
    // ALERT: Alice is eligible for the whole reward!
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount);
    // Bob's share: 50% in three rounds.
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), 3 * (rewardsAmount / 2));

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    // Alice got the entire reward of the third round.
    assertEq(alice.balance, 2 ether);
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 1 ether);

    // Bob tries to claim his reward and fails.
    vm.startPrank(bob);
    vm.expectRevert("Failed to transfer");
    vault.claimRewards(bob, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
}

Tools Used

Manual reward

Recommended Mitigation Steps

Consider this change:

--- a/contracts/liquid-staking/SyndicateRewardsProcessor.sol
+++ b/contracts/liquid-staking/SyndicateRewardsProcessor.sol
@@ -60,7 +60,7 @@ abstract contract SyndicateRewardsProcessor {
             // Calculate how much ETH rewards the address is owed / due
             uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
             if (due > 0) {
-                claimed[_user][_token] = due;
+                claimed[_user][_token] += due;

                 totalClaimed += due;

[c4-judge]

dmvt marked the issue as duplicate of #60

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as duplicate of #59

[C4-Staff]

JeeberC4 marked the issue as duplicate of #147