LSD Network - Stakehouse contest - joestakey's results

Lines of code

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

Vulnerability details

The burnLPToken of a Fees & MEV vault allow users to burn LP tokens in exchange of ETH.

Quoting the documentation

Every user has a right to leave the LSD network at anytime. A depositor/staker can simply sell their LP tokens to someone else or burn to redeem ETH or dETH

But there is a check in StakingFundsVault.sol reverting burnLPToken unless the Lifecycle of the validator is INITIALS_REGISTERED

180: require(
181:             getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
182:             "Cannot burn LP tokens"
183:         );

This means once the derivatives have been minted for a validator, a user will not be able to redeem their LP token for ETH. Even after staking withdrawal, when the lifecycle status has been updated to EXITED.

Impact

High

Proof of Concept

Add this test to StakingFundsVault.t.sol, showing that when the lifecycle is EXITED, the user cannot retrieve their ETH:

function testBurnLPRevert() public {
    // @audit - user deposits ETH in vault
    maxETHDeposit(accountFive, getBytesArrayFromBytes(blsPubKeyOne));

    // warp speed ahead
    vm.warp(block.timestamp + 3 hours);

    // @audit - Move lifecycle straight to exited 
    vault.accountMan().setLifecycleStatus(blsPubKeyOne, 4);

    vm.startPrank(accountFive);
    LPToken token = vault.lpTokenForKnot(blsPubKeyOne);

    // @audit - The call reverts
    vm.expectRevert("Cannot burn LP tokens");
    vault.burnLPForETH(token, maxStakingAmountPerValidator);
    vm.stopPrank();
}

You also need to make the following adjustments in the test file:

7: import {
8:     TestUtils,
9:     MockLSDNFactory,
10:     MockLiquidStakingManager,
11:     MockAccountManager,
+12:     MockStakingFundsVault,
13:     IDataStructures
14: } from "../utils/TestUtils.sol";
15: 
16: contract StakingFundsVaultTest is TestUtils {
17: 
18:     address operations = accountOne;
19: 
20:     MockLiquidStakingManager liquidStakingManager;
+21:     MockStakingFundsVault vault; //@audit - change to access account manager
22: 
23:     uint256 maxStakingAmountPerValidator;
...
...
52:     function setUp() public {
53:         factory = createMockLSDNFactory();
54:         liquidStakingManager = deployDefaultLiquidStakingNetwork(factory, admin);
55:         manager = liquidStakingManager;
+56:         vault = MockStakingFundsVault(payable(address(liquidStakingManager.stakingFundsVault()))); //@audit - vault 
57:         maxStakingAmountPerValidator = vault.maxStakingAmountPerValidator();

Tools Used

Manual Analysis, Foundry

Mitigation

The vault should allow users to redeem their LP tokens for ETH when the validator lifecycle has come to an end

```solidity
180: require(
-181:             getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
+181:             getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfKnot) == (IDataStructures.LifecycleStatus.INITIALS_REGISTERED || IDataStructures.LifecycleStatus.EXITED),
182:             "Cannot burn LP tokens"
183:         );

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L68 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L186

Vulnerability details

The burnLPToken of a protected vault allow users to burn LP tokens in exchange of ETH or dETH.

In the case of ETH, ie when the BLS key has not had its derivatives minted yet, the function checks the liquidity is not fresh by checking _lpToken.lastInteractedTimestamp(msg.sender)

140:         // before burning, check the last LP token interaction and make sure its more than 30 mins old before permitting ETH withdrawals
141:         bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp;

This opens a DOS attack vector: a user can prevent another from calling burnLPToken by transferring them any amount (even 1 gwei worth of LP), as the lastInteractedTimestamp of the receiver is updated in every LP._transfer():

66:     function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
67:         lastInteractedTimestamp[_from] = block.timestamp;
68:         lastInteractedTimestamp[_to] = block.timestamp; //@audit - recipient timestamp updated in every transfer

Note that the same grieving attack can be performed for a user trying to call rotateLPTokens().

The same issue is also present in savETHVault and the Giant pools.

Impact

Medium

Proof of Concept

• Alice and Bob both deposit 12 ETH in a protected vault, receiving LPTokens
• After some time, Alice decides to withdraw her ETH by calling burnLPToken()
• Bob front-runs her call with a LPToken.transfer(Alice, 1) call
• burnLPToken() will revert because Alice's liquidity is too fresh.

Add this test to SavETHVault.t.sol, recreating the steps described above:

function testBurnLPDOS() public {
      // First supply ETH when validator is at initials registered phase
      savETHVault.accountMan().setLifecycleStatus(blsPubKeyOne, 1);
      liquidStakingManager.setIsPartOfNetwork(blsPubKeyOne, true);
      
      uint256 stakeAmount = 12 ether;
      vm.deal(accountOne, stakeAmount);
      vm.deal(accountTwo, stakeAmount); 
      vm.warp(0);

      //@audit - accountOne and accountTwo both deposit 12ETH in the vault
      vm.prank(accountOne);
      savETHVault.depositETHForStaking{value: stakeAmount}(blsPubKeyOne, stakeAmount);
      vm.prank(accountTwo);
      savETHVault.depositETHForStaking{value: stakeAmount}(blsPubKeyOne, stakeAmount);

      LPToken lp = savETHVault.lpTokenForKnot(blsPubKeyOne);

      assertEq(lp.balanceOf(accountOne), 12 ether);

      skip(3600); //@audit - going 1 hour forward
      //@audit - accountTwo transfers 1 gwei worth of LP token to accountOne
      vm.prank(accountTwo);
      lp.transfer(accountOne, 1);


      //@audit - accountOne tries to redeem their LP for ETH
      // but because of the transfer that just happened, their LPtimestamp is too fresh
      vm.prank(accountOne);
      vm.expectRevert("Liquidity is still fresh");
      savETHVault.burnLPToken(lp, stakeAmount);
  }

Tools Used

Manual Analysis, Foundry

Mitigation

Not sure what viable measure can be taken here. Removing lastInteractedTimestamp[_to] = block.timestamp; from LPtoken._afterTokenTransfer is not viable, because it be make easy for users to transfer their LPtokens to another address and circumvent the 30 minutes requirement.

Judge has assessed an item in Issue #400 as M risk. The relevant finding follows:

L01 - EOA restriction of wallet representative can be bypassed https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L435

A node operator can call registerBLSPublicKeys() to register a node runner to LSD and create a new smart wallet. The protocol only allows EOAs to be registered as _eoaRepresentative.

The issue is that this can be easily circumvented. As detailed in the isContract() definition by OpenZeppelin:

• It is unsafe to assume that an address for which this function returns
• false is an externally-owned account (EOA) and not a contract.
• Among others, isContract will return false for the following
• types of addresses:
• • an externally-owned account
• • a contract in construction
• • an address where a contract will be created
• • an address where a contract lived, but was destroyed A user can for instance deploy a smart wallet via a factory using CREATE2, call SELFDESTRUCT on it. They can then pass the address of that destructed smart contract as _eoaRepresentative to registerBLSPublicKeys().

The operator can then redeploy the same contract to the same address using a CREATE2 call in their factory.

Impact Low

Judge has assessed an item in Issue #400 as M risk. The relevant finding follows:

L03 - LiquidStakingManager.dao can rug node operators with executeAsSmartWallet() 202: function executeAsSmartWallet( 203: address _nodeRunner, 204: address _to, 205: bytes calldata _data, 206: uint256 _value 207: ) external payable onlyDAO { 208: address smartWallet = smartWalletOfNodeRunner[_nodeRunner]; 209: require(smartWallet != address(0), "No wallet found"); 210: IOwnableSmartWallet(smartWallet).execute( 211: _to, 212: _data, 213: _value 214: ); 215: } executeAsSmartWallet() is meant to enable proxy operations through DAO contracts, but introduces rugging vectors.

Note that as per the docs

LSD Networks can be deployed by any Ethereum Account Despite the name, dao could be an EOA, making the attack even easier to perform.

A malicious dao can wait for a node operator to call registerBLSPublicKeys(), which will transfer amount = 4 ETH * Nvalidators to the smart wallet, then call executeAsSmartWallet() to transfer amount to their own account, rugging the node operator.

A bigger possible attack is rugging the staked ETH:

As per the logic in stake() the ETH staking deposit to the Ethereum Staking contract is performed by the node operator wallet.

When staking withdrawals are enabled, a dao can use executeAsSmartWallet() to withdraw all the staked ETH, rugging all the depositors.

Impact Centralization risk

Tools Used Manual Analysis

Judge has assessed an item in Issue #400 as M risk. The relevant finding follows:

L04 - LiquidStakingManager.dao can rug node operators with executeAsSmartWallet() daoCommissionPercentage is used to calculate the portion of node operator network rewards that are sent to dao, when a node runner is calling claimRewardsAsNodeRunner().

There is no timelock to updateDAORevenueCommission(), meaning a dao can call it with _commissionPercentage = MODULO to set daoCommissionPercentage to 100%, effectively stealing all the network rewards from node runners.

Impact Centralization risk

Tools Used Manual Analysis

Mitigation Consider either adding a timelock to updateDAORevenueCommission(), or add an upper boundary to it, so that daoCommissionPercentage is always reasonable.

QA Report

L01 - EOA restriction of wallet representative can be bypassed

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

A node operator can call registerBLSPublicKeys() to register a node runner to LSD and create a new smart wallet. The protocol only allows EOAs to be registered as _eoaRepresentative.

The issue is that this can be easily circumvented. As detailed in the isContract() definition by OpenZeppelin:

* It is unsafe to assume that an address for which this function returns
* false is an externally-owned account (EOA) and not a contract.
*
* Among others, `isContract` will return false for the following
* types of addresses:
*
*  - an externally-owned account
*  - a contract in construction
*  - an address where a contract will be created
*  - an address where a contract lived, but was destroyed

A user can for instance deploy a smart wallet via a factory using CREATE2, call SELFDESTRUCT on it. They can then pass the address of that destructed smart contract as _eoaRepresentative to registerBLSPublicKeys().

The operator can then redeploy the same contract to the same address using a CREATE2 call in their factory.

Impact

Low

Tools Used

Manual Analysis

Mitigation

A potential mitigation would be to change the logic so that the _eoaRepresentative would need to call registerBLSPublicKeys(), and use a msg.sender == tx.origin check to ensure they are an EOA.

L02 - Lack of coherence for the wallet representative

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L435 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L290-L296 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L309-L320

A node operator can call registerBLSPublicKeys() to register a node runner to LSD and create a new smart wallet. The protocol only allows EOAs to be registered as _eoaRepresentative.

But in rotateEOARepresentativeOfNodeRunner() and rotateEOARepresentative(), there is no such check meaning, _newRepresentative can be a smart contract.

Note that the error strings here and here and the function NATSPEC comment explicitly indicate the representative should be an EOA.

Impact

Low

Tools Used

Manual Analysis

Mitigation

Consider using an Address.isContract() check in both functions, but as stated in L01 this does not really mitigate the issue properly. Another option would be use a two-step rotation, with the current representative setting the pending new representative, and the new representative would call the function setting them as the smart wallet representative - allowing the use of msg.sender == tx.origin to ensure they are an EOA.

L03 - LiquidStakingManager.dao can rug node operators with executeAsSmartWallet()

202: function executeAsSmartWallet(
203:         address _nodeRunner,
204:         address _to,
205:         bytes calldata _data,
206:         uint256 _value
207:     ) external payable onlyDAO {
208:         address smartWallet = smartWalletOfNodeRunner[_nodeRunner];
209:         require(smartWallet != address(0), "No wallet found");
210:         IOwnableSmartWallet(smartWallet).execute(
211:             _to,
212:             _data,
213:             _value
214:         );
215:     }

executeAsSmartWallet() is meant to enable proxy operations through DAO contracts, but introduces rugging vectors.

Note that as per the docs

LSD Networks can be deployed by any Ethereum Account

Despite the name, dao could be an EOA, making the attack even easier to perform.

A malicious dao can wait for a node operator to call registerBLSPublicKeys(), which will transfer amount = 4 ETH * Nvalidators to the smart wallet, then call executeAsSmartWallet() to transfer amount to their own account, rugging the node operator.

A bigger possible attack is rugging the staked ETH:

As per the logic in stake() the ETH staking deposit to the Ethereum Staking contract is performed by the node operator wallet.

When staking withdrawals are enabled, a dao can use executeAsSmartWallet() to withdraw all the staked ETH, rugging all the depositors.

Impact

Centralization risk

Tools Used

Manual Analysis

L04 - LiquidStakingManager.dao can rug node operators with executeAsSmartWallet()

daoCommissionPercentage is used to calculate the portion of node operator network rewards that are sent to dao, when a node runner is calling claimRewardsAsNodeRunner().

There is no timelock to updateDAORevenueCommission(), meaning a dao can call it with _commissionPercentage = MODULO to set daoCommissionPercentage to 100%, effectively stealing all the network rewards from node runners.

Impact

Centralization risk

Tools Used

Manual Analysis

Mitigation

Consider either adding a timelock to updateDAORevenueCommission(), or add an upper boundary to it, so that daoCommissionPercentage is always reasonable.

L05 - LiquidStakingManager.receive can lead to user losing funds

The receive() function is empty. If an account mistakenly sends ETH to a LiquidStakingManager, they have no way to retrieve that amount.

It will later be taken into account for rewards calculation here

Impact

Low

Tools Used

Manual Analysis

Mitigation

The LiquidStakingManager.receive() function should check msg.sender to ensure the call was triggered by a rightful address to ensure they are staking yield/rewards, and not an arbitrary transfer from a random EOA.