StakingFundsVault token transfer causes vault DOS and orphaned rewards for token sender.

[code423n4]

Lines of code

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

Vulnerability details

Impact

StakingFundsVault's afterTokenTransfer() does not update claimed[] on the from side of the token transfer. This can result in a claimed[] value that is too high for the new (lower) number of tokens held on that side. It can result in DOS when the user tries to access the vault in a way that calls _distributeETHRewardsToUserForToken() which includes transferring tokens or claiming rewards. (This function will underflow when subtracting claimed[].) The overstated claimed[] value can also cause rewards due to the user to be orphaned in the vault.

Proof of Concept

The POC demonstrates how transferring StakingFundsVault tokens causes dos accessing the vault, including when attempting further token transfers. And then shows how future rewards can be orphaned.
The POC patch includes a temp fix for the claimed[] calculation in _distributeETHRewardsToUserForToken() to demonstrate that this is a separate issu.

Test

forge test -m testTransferTokenCausesDOSAndOrphanedRewards

Patch

diff --git a/contracts/liquid-staking/SyndicateRewardsProcessor.sol b/contracts/liquid-staking/SyndicateRewardsProcessor.sol
index 81be706..ca44ae6 100644
--- 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; // temp fix claimed calculation
 
                 totalClaimed += due;
 
diff --git a/test/foundry/StakingFundsVault.t.sol b/test/foundry/StakingFundsVault.t.sol
index 53b4ce0..b307e30 100644
--- a/test/foundry/StakingFundsVault.t.sol
+++ b/test/foundry/StakingFundsVault.t.sol
@@ -417,4 +417,65 @@ contract StakingFundsVaultTest is TestUtils {
         assertEq(vault.totalClaimed(), rewardsAmount);
         assertEq(vault.totalRewardsReceived(), rewardsAmount);
     }
+
+    function testTransferTokenCausesDOSAndOrphanedRewards() 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, getUint256ArrayFromValues(maxStakingAmountPerValidator), 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 / 2);
+        assertEq(vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)), 0);
+
+        // 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 / 2);
+        assertEq(address(vault).balance, 0);
+
+        // accountTwo's claimed[] value is correct.
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 2);
+
+        // accountTwo transfers half of its tokens to accountOne.
+        vm.prank(accountTwo);
+        lpTokenBLSPubKeyFour.transfer(accountOne, maxStakingAmountPerValidator / 2);
+        assertEq(lpTokenBLSPubKeyFour.balanceOf(accountTwo), maxStakingAmountPerValidator / 2);
+        assertEq(lpTokenBLSPubKeyFour.balanceOf(accountOne), maxStakingAmountPerValidator / 2);
+
+        // accountTwo's claimed[] value is unchanged, despite its lower token balance.
+        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), totalRewardAmount / 2);
+
+        // accountTwo experiences DOS for some actions including checking accumulated ETH, transferrig
+        // tokens, others.
+        vm.prank(accountTwo);
+        vm.expectRevert();
+        vault.previewAccumulatedETH(accountTwo, lpTokenBLSPubKeyFour);
+
+        vm.prank(accountTwo);
+        vm.expectRevert();
+        lpTokenBLSPubKeyFour.transfer(accountOne, 0.1 ether);
+
+        // Deal remaining rewards into the vault.
+        vm.deal(address(vault), totalRewardAmount / 2);
+
+        // accountOne receives the proper rewards, but accountTwo receives no rewards due to the
+        // incorrect claimed[] value. The missing rewards are orphaned in the vault.
+        assertEq(vault.previewAccumulatedETH(accountOne, lpTokenBLSPubKeyFour), totalRewardAmount / 4);
+        assertEq(vault.previewAccumulatedETH(accountTwo, lpTokenBLSPubKeyFour), 0);
+  }
+
 }
\ No newline at end of file

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Correctly set claimed[] when transferring away StakingFundsVault tokens. Alternatively, consider tracking claimed[] on a per share rather than an absolute basis.

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt marked the issue as duplicate of #178

[c4-judge]

dmvt marked the issue as nullified