merlin - PufETHAdapter does not handle the case when the stakeLimit of stETH is zero correctly

[sherlock-admin4]

merlin

medium

PufETHAdapter does not handle the case when the stakeLimit of stETH is zero correctly

Summary

PufETHAdapter does not handle the case when the stakeLimit of stETH is zero correctly.

/// @dev Need to check the current staking limit before staking to prevent DoS.
    function _stake(uint256 stakeAmount) internal override returns (uint256) {

Vulnerability Detail

If STETH.getCurrentStakeLimit() returns a zero value, then stakeAmount will also be 0, which will lead to a call to the STETH.submit function with zero msg.value, and the transaction will revert with ZERO_DEPOSIT reason.

function _stake(uint256 stakeAmount) internal override returns (uint256) {
        if (stakeAmount == 0) return 0;

        uint256 stakeLimit = STETH.getCurrentStakeLimit();
        if (stakeAmount > stakeLimit) { // 1 WETH > 0
            // Cap stake amount
            stakeAmount = stakeLimit; // stakeAmount = 0
        }

        IWETH9(Constants.WETH).withdraw(stakeAmount);
        uint256 _stETHAmt = STETH.balanceOf(address(this));
        STETH.submit{value: stakeAmount}(address(this)); // tx revert

        ///code
}

The submit function of stETH:

function _submit(address _referral) internal returns (uint256) {
        require(msg.value != 0, "ZERO_DEPOSIT");

Impact

The transaction should not fail but instead increase the buffer on availableEth.

Code Snippet

src/adapters/puffer/PufETHAdapter.sol#L72

Tool used

Manual Review

Recommendation

Consider returning a zero value if stakeLimit equals 0:

uint256 stakeLimit = STETH.getCurrentStakeLimit();
        if (stakeAmount > stakeLimit) {
            // Cap stake amount
            stakeAmount = stakeLimit;
        }
+ if (stakeAmount == 0) return 0;        

Duplicate of #75

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/napierfi/napier-uups-adapters/pull/14

[foufrix]

Escalate

Duplicate of https://github.com/sherlock-audit/2024-05-napier-update-judging/issues/75.

The issue is the same when stakeLimit is 0 the transaction will fail (DoS).

The fix is the same, aka move the line if (stakeAmount == 0) return 0; after the call of stakeLimit, so that it returns 0 and does not revert in case stakeLimit is 0.

It's in a different file but impacts the same function, _stake, thus the same logic

Per Sherlock docs:

In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates. The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

Already discussed with @z3s on Discord and agreed on that.

[sherlock-admin3]

Escalate

Duplicate of https://github.com/sherlock-audit/2024-05-napier-update-judging/issues/75.

The issue is the same when stakeLimit is 0 the transaction will fail (DoS).

The fix is the same, aka move the line if (stakeAmount == 0) return 0; after the call of stakeLimit, so that it returns 0 and does not revert in case stakeLimit is 0.

It's in a different file but impacts the same function, _stake, thus the same logic

Per Sherlock docs:

In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates. The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

Already discussed with @z3s on Discord and agreed on that.

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.

[z3s]

I don't completely agree that these could be duplicated because there are a couple of _stake functions to interact with other protocols. These two have almost the same implementation, but not exactly the same. I accept what the Internal Judge decides.

A Question for the Internal Judge, If the sponsor decides not to check closed issues (invalids and duplicates), like in this contest, and if I duplicated these issues, one of them might be missed. Do you suggest editing one of the reports to add a warning about the duplicate so it gets seen by the sponsor?

[WangSecurity]

I agree with the escalation that these should be duplicates, based on the quoted Rule specifically. Indeed, this issue arises in different contracts, but it's the same core issue, with the same fix and the same impact.

@z3s regarding your question, the best way in these cases is just flag to sponsors that these are duplicates according to the rules, but require fixes separately.

Planning to accept the escalation and duplicate with #75

[Evert0x]

Result: Medium Duplicate of #75

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• foufrix: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.