LSD Network - Stakehouse contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/StakingFundsVault.sol#L337

Vulnerability details

Impact

StakingFundsVault.beforeTokenTransfer only checks if the _from address is not equal to address(0), but does not perform the same check for the _to address:

// distribute any due rewards for the `from` user
if (_from != address(0)) {
    _distributeETHRewardsToUserForToken(_from, address(token), token.balanceOf(_from), _from);
}

// in case the new user has existing rewards - give it to them so that the after transfer hook does not wipe pending rewards
_distributeETHRewardsToUserForToken(_to, address(token), token.balanceOf(_to), _to);

This can be very problematic when tokens are burned. Then, StakingFundsVault.beforeTokenTransfer will be called with a _recipient address (fourth parameter) of address(0) and this address will be passed to _distributeETHRewardsToUserForToken. However, this function reverts when the _to address is zero:

require(_recipient != address(0), "Zero address");

Therefore, the burn operation fails.

Proof Of Concept

This behavior can be seen in the test testBurnLPForETHWorksAsExpected:

│   │   ├─ [6740] LPToken::burn(0x2546BcD3c84621e976D8185a91A922aE77ECEc30, 4000000000000000000) [delegatecall]
    │   │   │   ├─ [1791] MockStakingFundsVault::beforeTokenTransfer(0x2546BcD3c84621e976D8185a91A922aE77ECEc30, 0x0000000000000000000000000000000000000000, 4000000000000000000) 
    │   │   │   │   ├─ [636] 0xCF8cBf15457977aeCe0e243c2584B038B6Ac6933::syndicate() [staticcall]
    │   │   │   │   │   ├─ [470] MockLiquidStakingManager::syndicate() [delegatecall]
    │   │   │   │   │   │   └─ ← 0x0000000000000000000000000000000000000000

StakingFundsVault.beforeTokenTransfer is called with address(0) as the second parameter. However, because syndicate is set to address(0) in this test, the logic is not triggered and the test still passes.

Recommended Mitigation Steps

Only call _distributeETHRewardsToUserForToken when _to is not zero.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L79

Vulnerability details

Impact

SyndicateRewardsProcessor._updateAccumulatedETHPerLP performs the following calculation:

uint256 unprocessed = received - totalETHSeen;

received is the result of totalRewardsReceived(), i.e. the following:

function totalRewardsReceived() public virtual view returns (uint256) {
        return address(this).balance + totalClaimed;
}

totalETHSeen is set to the received value of the last _updateAccumulatedETHPerLP call.

The problem with this logic is that it assumes that totalRewardsReceived() is increasing, which is not always the case. When ETH is withdrawn using StakingFundsVault.withdrawETH, the ETH balance decreases, while totalClaimed does not increase (which it does when ETH is transferred in burnLPForETH).

Proof Of Concept

As described above, the problem can occur after stake operations when the ETH balance is reduced. When this happens, all succesive calls to _updateAccumulatedETHPerLP (which is called often in StakingFundsVault) will revert.

Recommended Mitigation Steps

Handle the case of ETH withdrawals correctly

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantMevAndFeesPool.sol#L42 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantSavETHVaultPool.sol#L48

Vulnerability details

Impact

In batchDepositETHForStaking, the user provides the addresses of the staking funds vaults in the array _stakingFundsVault. The system then checks that the liquid staking network manager that is returned by this vault is registered. If this check succeeds, the requested ETH amount is sent to the provided address (with a call to batchDepositETHForStaking):

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

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

An attacker can exploit this by providing a malicious fake staking funds vault. This smart contract returns a valid liquid staking manager address when liquidStakingNetworkManager() is called. However, when batchDepositETHForStaking is called, it simply consumes all the ETH.

Proof Of Concept

A contract to exploit this vulnerability could look like this:

contract AttackerContract {
	function liquidStakingNetworkManager() public view returns (address) {
		return validLiquidStakingManager; // Some stored valid liquid staking manager address
	}

	function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) payable public {
		// Do nothing, just accept the ETH
	}
}

The attacker would deploy this contract and pass the address in the _stakingFundsVault array. Furthermore, he would pass in the whole ETH balance in the corresponding _ETHTransactionAmounts entry to drain the contract.

Recommended Mitigation Steps

Validate that the provided vault is a registered vault.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantLP.sol#L45 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/LPToken.sol#L68 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantPoolBase.sol#L96 https://github.com/code-423n4/2022-11-stakehouse/blob/a0558ed7b12e1ace1fe5c07970c7fc07eb00eebd/contracts/liquid-staking/ETHPoolLPFactory.sol#L82

Vulnerability details

Impact

After a token transfer, lastInteractedTimestamp[_to] is set to the current block timestamp. The system checks this timestamp in multiple places to ensure that the most recent transfer / interaction is not too recent. For instance, in GiantPoolBase._assertUserHasEnoughGiantLPToClaimVaultLP, the following check is performed:

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

This can be exploited by an attacker to grief (and potentially blackmail) users: Whenever a user wants to perform an action with such a check, the attacker transfers an extremely small amount (1 wei) of his LP tokens to this user such that his lastInteractedTimestamp is reset. Like that, the action always fails.

Proof Of Concept

Alice wants to withdraw vault LP tokens by burning her giant LP tokens and calls withdrawLPTokens to do so. The attacker Charlie frontsrun this call and transfers 1 Wei of lpTokenETH to Alice before the call. Because of this, the withdrawal will fail. An attacker can repeat this attack multiple times to prevent all withdrawals.

Recommended Mitigation Steps

Only set lastInteractedTimestamp when the amount that was transferred is greater than some minimum amount.

Lines of code

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

Vulnerability details

Impact

In registerBLSPublicKeys, it should be checked (according to the comment and error) if a BLS public key is part of the LSD network and not banned:

// check if the BLS public key is part of LSD network and is not banned
require(isBLSPublicKeyPartOfLSDNetwork(_blsPublicKey) == false, "BLS public key is banned or not a part of LSD network");

However, this is not actually checked. The function isBLSPublicKeyPartOfLSDNetwork only checks if the public key is part of the LSD network:

function isBLSPublicKeyPartOfLSDNetwork(bytes calldata _blsPublicKeyOfKnot) public virtual view returns (bool) {
        return smartWalletOfKnot[_blsPublicKeyOfKnot] != address(0);
}

The function isBLSPublicKeyBanned would perform both checks and should be called here:

function isBLSPublicKeyBanned(bytes calldata _blsPublicKeyOfKnot) public virtual view returns (bool) {
        return !isBLSPublicKeyPartOfLSDNetwork(_blsPublicKeyOfKnot) || bannedBLSPublicKeys[_blsPublicKeyOfKnot] != address(0);
}

Because of that, it is possible to pass banned BLS public keys to registerBLSPublicKeys and the call will succeed.

Recommended Mitigation Steps

Use isBLSPublicKeyBanned instead of isBLSPublicKeyPartOfLSDNetwork.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/a0558ed7b12e1ace1fe5c07970c7fc07eb00eebd/contracts/liquid-staking/LiquidStakingManager.sol#L882 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/syndicate/Syndicate.sol#L22

Vulnerability details

Impact

LiquidStakingManager._autoStakeWithSyndicate always stakes a fixed amount of 12 ETH. However, Syndicate.stake only allows a total staking amount of 12 ETH and reverts otherwise:

if (_sETHAmount + totalStaked > 12 ether) revert InvalidStakeAmount();

An attacker can abuse this and front-run calls to mintDerivatives (which call _autoStakeWithSyndicate internally). Because Syndicate.stake can be called by everyone, he can stake the minimum amount (1 gwei) such that the mintDerivatives call fails.

Proof Of Concept

As soon as there is a mintDerivatives call in the mempool, an attacker (that owns sETH) calls Syndicate.stake with an amount of 1 gwei. _autoStakeWithSyndicate will still call Syndicate.stake with 12 ether. However, _sETHAmount + totalStaked > 12 ether will then be true, meaning that the call will revert.

Recommended Mitigation Steps

Only allow staking through the LiquidStakingManager, i.e. add access control to Syndicate.stake.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantPoolBase.sol#L96 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/liquid-staking/GiantSavETHVaultPool.sol#L87

Vulnerability details

Impact

The loop within GiantPoolBase.withdrawLPTokens calls the function _assertUserHasEnoughGiantLPToClaimVaultLP for every token in the _lpTokens array. This function checks that lpTokenETH.lastInteractedTimestamp(msg.sender) is older than 1 day. This logic works fine when only one token is in the _lpTokens array. However, when it contains multiple ones, the withdrawal will fail. This happens because the lpTokenETH.burn(msg.sender, amount) call (in the first loop iteration) resets the lastInteractedTimestamp for msg.sender, as a burn also triggers GiantLP._afterTokenTransfer, which sets these values:

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, all burns where multiple LP tokens are provided fail.

Note that this problem is also present in GiantSavETHVaultPool.

Proof Of Concept

A user calls withdrawLPTokens and passes two LP tokens in the _lpTokens array. lpTokenETH.lastInteractedTimestamp(msg.sender) is older than 1 day. However, after the first lpTokenETH.burn(msg.sender, amount) call, it will be set to the current block.timestamp, meaning that the second time _assertUserHasEnoughGiantLPToClaimVaultLP is called, this check fails (although it should not):

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

Recommended Mitigation Steps

Only check once if lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/a0558ed7b12e1ace1fe5c07970c7fc07eb00eebd/contracts/liquid-staking/ETHPoolLPFactory.sol#L111 https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/syndicate/Syndicate.sol#L215

Vulnerability details

Impact

In ETHPoolLPFactory._depositETHForStaking, a minimum deposit amount (set to 0.001 ether) is required:

require(_amount >= MIN_STAKING_AMOUNT, "Min amount not reached");

On the other hand, the maximum amount is also restricted such that maxStakingAmountPerValidator will not be exceeded:

require(lpToken.totalSupply() + _amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator");

This combination of checks can lead to situations where maxStakingAmountPerValidator is never reachable, namely when lpToken.totalSupply() > maxStakingAmountPerValidator - MIN_STAKING_AMOUNT. Every deposit that is larger than MIN_STAKING_AMOUNT will revert because it would exceed the maximum staking amount and every deposit that is smaller reverts because this is the enforced minimum.

Note that the same problem also can occur in Syndicate.stake where a minimum stake amount of 1 gwei is enforced, but the total staked amount cannot exceed 12 ether.

Proof Of Concept

Let's say that a StakingFundsVault for a given BLS public key has already 4 - 0.001 ether and 1 wei. When a user now wants to deposit < 0.001 ether, the call reverts because the minimum amount is not reached. For more than 0.001 ether, the call reverts because this would exceed the staking limit.

Recommended Mitigation Steps

Do not allow deposits that bring the total supply into the range [maxStakingAmountPerValidator - MIN_STAKING_AMOUNT, maxStakingAmountPerValidator - 1].