code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Attacker can take away all eth in the pools.

Proof of Concept

Both GiantSavETHVaultPool and GiantMevAndFeesPool provide a function - batchDepositETHForStaking to deposit ETH from pool to vault for staking.

The main logic is:

for (uint256 i; i < numOfSavETHVaults; ++i) {
    uint256 transactionAmount = _ETHTransactionAmounts[i];

    // As ETH is being deployed to a savETH pool vault, it is no longer idle
    idleETH -= transactionAmount;

    SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
    require(
        liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
        "Invalid liquid staking manager"
    );

    // Deposit ETH for staking of BLS key
    savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
        _blsPublicKeys[i],
        _stakeAmounts[i]
    );
}

It will check the vault by the liquidStakingManager address, and then send ETH to the vault by calling batchDepositETHForStaking.

Attacker can deploy a mock vault contract (M) which implements following two functions:

function isLiquidStakingManager() returns the correct liquidStakingManager address.
functoin batchDepositETHForStaking(bytes[], uint256[]) payable to receive ETH.

Then the attacker can call batchDepositETHForStaking of the pools with the mock vault contract M, and ETH in pools will be sent to M directly.

Tools Used

n/a

Recommended Mitigation Steps

We need check the vault addresses passed to batchDepositETHForStaking, not to check the liquidStakingManager returned by the vault contract.

An easy way to do this is to register all newly created valid vault contracts in a trusted contract like the liquidStakingDerivativeFactory(LSDNFactory), and then check within the batchDepositETHForStaking to see if vault is in the registry.