search

sherlock-audit / 2024-01-napier-judging

9 stars 6 forks source link

Bandit - Attacker Can Trigger Lido Staking Limit To Steal Lido Eth Tranche Deposit Attempts #59

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 7 months ago

Bandit

high

Attacker Can Trigger Lido Staking Limit To Steal Lido Eth Tranche Deposit Attempts

Summary

When staking limit is reached, depositor loses excess prefunded WETH without getting corresponding shares or PT/YT in return.

Vulnerability Detail

When calling issue through a Tranche with a Lido adapter, WETH is sent to the adapter, and prefundedDeposit is called, which calls _stake. In the _stake function, not all of the predeposited WETH is staked if it would cause the stakingLimit to be exceeded:

    function _stake(uint256 stakeAmount) internal override returns (uint256) {
        uint256 stakeLimit = STETH.getCurrentStakeLimit();
        if (stakeAmount > stakeLimit) {
            // Cap stake amount
            stakeAmount = stakeLimit;
        }

The problem is that the WETH that isn't staked within the _stake function is not refunded to user. Since the user only gets shares (or PT/YT) based on the stakedAmount, the excess WETH is entirely lost for them. Instead, it is added to the bufferEth which inflates the value of the totalAssets() and hence the share to asset conversion rate. This benifits all pre-existing share owners.

An attacker has an incentive to cause this scenario as they can capture the lost WETH as profit using this sequence:

  1. mint themselves a large amount of shares or PT/YT. This is to capture any value that comes through boosting the share-to-asset conversion ratio
  2. Stake large amount of ETH in Lido to approach staking limit.
  3. Victim tries to issue themselves PT/YT but they reach staking limit resulting in only partial stake.
  4. The excess stake stays in adapater and is attributed to bufferEth which infaltes share price.
  5. The attacker captures the share price inflation as profit due to step 1.

Impact

Depositor loses excess WETH due to exceeding staking limit. Attacker is incentivised to cause this as they can profit off the victim's loss.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/adapters/lido/StEtherAdapter.sol#L67-L79

Tool used

Manual Review

Recommendation

When stakingAmount is less than assets in prefundedDeposit, send the excess assets back to msg.sender, or the original caller of issue in the case where it was called via the Tranche contract.

sherlock-admin commented 7 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

It would be user mistake not to check how much is stakeLimit and stake more than the limit

takarez commented:

valid: mitigations required; medium(9)

BanditSecurity commented 7 months ago

Escalate.

This is valid, and the correctly reverting transaction flow involves calling deposit in Lido rather than stake.

Regarding @stsevanovv comment, the stakeLimit can always be triggered by an attacker by frontrunning the issue transaction with a large stake, which is not the user's mistake. This take a large amount of capital but is also "costless", in the sense that the ETH can be unstaked after the attack without loss.

sherlock-admin2 commented 7 months ago

Escalate.

This is valid, and the correctly reverting transaction flow involves calling deposit in Lido rather than stake.

Regarding @stsevanovv comment, the stakeLimit can always be triggered by an attacker by frontrunning the issue transaction with a large stake, which is not the user's mistake. This take a large amount of capital but is also "costless", in the sense that the ETH can be unstaked after the attack without loss.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

nevillehuang commented 7 months ago

Escalate

Agree with @BanditSecurity second point, this issue is a valid medium severity issue given the possibility of front-run

sherlock-admin2 commented 7 months ago

Escalate

Agree with @BanditSecurity second point, this issue is a valid medium severity issue given the possibility of front-run

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

massun-onibakuchi commented 7 months ago

@nevillehuang @nevillehuang

The problem is that the WETH that isn't staked within the _stake function is not refunded to user. Since the user only gets shares (or PT/YT) based on the stakedAmount, the excess WETH is entirely lost for them. Instead, it is added to the bufferEth which inflates the value of the totalAssets() and hence the share to asset conversion rate. This benifits all pre-existing share owners.

These core statements are totally wrong. This issue is invalid.

Since the user only gets shares (or PT/YT) based on the stakedAmount,

Correct: Since the user only gets shares (or PT/YT) based on the the deposit.

the excess WETH is entirely lost for them. Instead, it is added to the bufferEth which inflates the value of the totalAssets() and hence the share to asset conversion rate.

Correct: Not lost. It doesn't inflates the asset conversion rate.

    function prefundedDeposit() external nonReentrant returns (uint256, uint256) {
        uint256 bufferEthCache = bufferEth; // cache storage reads
        uint256 queueEthCache = withdrawalQueueEth; // cache storage reads
        uint256 assets = IWETH9(WETH).balanceOf(address(this)) - bufferEthCache; // amount of WETH deposited at this time
        uint256 shares = previewDeposit(assets);
        .......
        /// WRITE ///
        _mint(msg.sender, shares);

Please provide a PoC to show the mentioned impact.

Banditx0x commented 7 months ago

@massun-onibakuchi I'm not sure how you could say this issue is invalid when there is we both agree that:

What I meant by "inflate" is that since BufferEth increases, then there is a larger amount of recorded assets which in turn increases the assetToShare ratio. This is not related to the "share inflation" attack.

It is not the depositors fault if their prefunded amount is not entirely staked when an attacker can front-run them to hit the Lido staking limit to induce a partial stake. Like you said: "the user only gets shares (or PT/YT) based on the the deposit". Since the ETH deposited is less than the ETH the user who called issue sent, this is an unfair loss for them.

xiaoming9090 commented 6 months ago

The report mentioned that:

The problem is that the WETH that isn't staked within the _stake function is not refunded to user. Since the user only gets shares (or PT/YT) based on the stakedAmount, the excess WETH is entirely lost for them.

However, I don't see how the above statement is true, similar to the sponsor's comment (https://github.com/sherlock-audit/2024-01-napier-judging/issues/59#issuecomment-1986789783) above. The number of shares the users will get is based on the previewDeposit function at Line 75 below. It has nothing to do with the stakeAmount at Line 133 below. Regardless of the stakeAmount returned at Line 133, the amount of shares that need to be minted to the depositor based on their deposited assets is already executed at Line 103 below.

File: BaseLSTAdapter.sol
071:     function prefundedDeposit() external nonReentrant returns (uint256, uint256) {
072:         uint256 bufferEthCache = bufferEth; // cache storage reads
073:         uint256 queueEthCache = withdrawalQueueEth; // cache storage reads
074:         uint256 assets = IWETH9(WETH).balanceOf(address(this)) - bufferEthCache; // amount of WETH deposited at this time
075:         uint256 shares = previewDeposit(assets);
..SNIP..
102:         /// WRITE ///
103:         _mint(msg.sender, shares);
..SNIP..
130:         /// INTERACT ///
131:         // Deposit into the yield source
132:         // Actual amount of ETH spent may be less than the requested amount.
133:         stakeAmount = _stake(stakeAmount); // stake amount can be 0
134: 
135:         /// WRITE ///
136:         bufferEth = (availableEth - stakeAmount).toUint128(); // no underflow theoretically
137: 
138:         return (assets, shares);
139:     }
cvetanovv commented 6 months ago

I want to comment on the deposit limit, which is the core of the report. In theory, what @BanditSecurity says could happen. Initially, I have excluded the issue because it would be a mistake for a user not to see that the limit has been reached and stake eth.

But what the escalation says that someone could frontrun a user is possible in theory, but what is the likelihood?

Czar102 commented 6 months ago

If there is an issue, I'm inclined to accept this as a valid Medium. But that will require @Banditx0x to answer concerns from https://github.com/sherlock-audit/2024-01-napier-judging/issues/59#issuecomment-1994635393.

Banditx0x commented 6 months ago

Hey I realised that @xiaoming9090 and Sponsor are correct. I misunderstood the purpose of BufferEth and the effect it will have on balances. Leaning towards my own issue being invalid.

Czar102 commented 6 months ago

Planning to reject the escalations and leave the issue as is. cc @nevillehuang

Czar102 commented 6 months ago

Result: Invalid Unique

sherlock-admin3 commented 6 months ago

Escalations have been resolved successfully!

Escalation status: