search

sherlock-audit / 2024-05-napier-update-judging

8 stars 7 forks source link

merlin - The scenario where the buffer in RsETHAdapter can be much larger than expected #50

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

merlin

medium

The scenario where the buffer in RsETHAdapter can be much larger than expected

Summary

There is a scenario where the buffer value can exceed the targetBufferEth or even rsETH value in ETH, which would result in eRsETH becoming less valuable and earning fewer rewards.

Vulnerability Detail

RsETHAdapter

Currently, the maximum amount of ETH that all users can deposit in Kelp DAO is 100,000 ETH, with the current limit around 96,000 ETH. This limit is set by Kelp DAO, and if all users deposit 100,000 ETH, it will not be possible to make any more deposits. The 100,000 ETH max stake limit is a restriction for all users collectively, not for a single address. minAmountToDeposit = 0.0001 ETH

/// @notice gets the current limit of asset deposit
    /// @param asset Asset address
    /// @return currentLimit Current limit of asset deposit
    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
        uint256 totalAssetDeposits = getTotalAssetDeposits(asset);
        if (totalAssetDeposits > lrtConfig.depositLimitByAsset(asset)) {
            return 0;
        }

        return lrtConfig.depositLimitByAsset(asset) - totalAssetDeposits;
    }

Prerequisite: 50 WETH in the Kelp DAO protocol from RsETHAdapter Suppose the current stake limit of the LRTDepositPool is 0.1 ETH, but a user deposits 100 WETH, resulting in an increase in the buffer to 99.9 WETH. The WETH tokens held in the RsETHAdapter smart contract will not earn rewards, leading to users receiving lower rewards.

// Check LRTDepositPool stake limit
        uint256 stakeLimit = RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH);
        if (stakeAmount > stakeLimit) { // 100 WETH > 0.1 WETH
            // Cap stake amount
            stakeAmount = stakeLimit; // stakeAmount = 0.1 WETH
        }

If the buffer is insufficient, then the buffer must increase. However, in this report, I highlighted an issue where depositors of eRsETH will start earning fewer rewards because significantly more WETH will be in the buffer than necessary. In this specific case, since Kelp DAO has a max staking limit, any excess WETH that exceeds this limit should not be stored in the buffer.

Impact

Users will earn rewards that are lower than expected.

Code Snippet

src/adapters/kelp/RsETHAdapter.sol#L71-L77

Tool used

Manual Review

Recommendation

In this case, the transaction should fail if the stakeAmount exceeds the current rsETH limit:

uint256 stakeLimit = RSETH_DEPOSIT_POOL.getAssetCurrentLimit(Constants.ETH);
        if (stakeAmount > stakeLimit) {
            // Cap stake amount
-            stakeAmount = stakeLimit;
+           revert Error;
        }

In this specific case, since Kelp DAO has a max staking limit, any excess WETH that exceeds this limit should not be stored in the buffer and instead result in a transaction revert.

massun-onibakuchi commented 3 months ago

rsETHAdapter have functions to set max stake limit, which would prevent such situation.

merlin-up commented 3 months ago

Escalate I agree with @massun-onibakuchi that changing the max stake limit in RsETHAdapter will prevent such an issue, and it is a better solution than my recommendation.

However, this issue was not included in the known issues section, also to change the max stake limit in RsETHAdapter, you need to know the limit in the RSETH_DEPOSIT_POOL smart contract, which can change over time. The README indicates that there is no off-chain bot monitoring the limit in RSETH_DEPOSIT_POOL. Therefore, I believe the scenario outlined in the report could occur, leading to a change in the ratio of [WETH in RsETHAdapter : WETH in RSETH_DEPOSIT_POOL]. Consequently, stakers will start receiving fewer rewards, making it less advantageous for them to continue holding PT + YT of RsETHAdapter.

From the README: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?

sherlock-admin3 commented 3 months ago

Escalate I agree with @massun-onibakuchi that changing the max stake limit in RsETHAdapter will prevent such an issue, and it is a better solution than my recommendation.

However, this issue was not included in the known issues section, also to change the max stake limit in RsETHAdapter, you need to know the limit in the RSETH_DEPOSIT_POOL smart contract, which can change over time. The README indicates that there is no off-chain bot monitoring the limit in RSETH_DEPOSIT_POOL. Therefore, I believe the scenario outlined in the report could occur, leading to a change in the ratio of [WETH in RsETHAdapter : WETH in RSETH_DEPOSIT_POOL]. Consequently, stakers will start receiving fewer rewards, making it less advantageous for them to continue holding PT + YT of RsETHAdapter.

From the README: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?

  • We monitor token balances on adapters, available buffer on adapters and current buffer percentage. And we may run bots to rebalance vaults when users run out of buffer.

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.

WangSecurity commented 3 months ago

Firstly, as we all agree, the stake limit in RsETHAdapter can be changed by an admin. It requires to know the correct stake limit inside the RSETH_DEPOSIT_POOL, which I believe will be known by the TRUSTED admin and tracked correctly.

Still, if the stake limit of RSETH_DEPOSIT_POOL is full and wasn't increased, it may lead to many funds stack up in the adapter as buffer. I believe it's design decision that allows for this behaviour. Yes, the users may receive less rewards, but it's an opportunity loss and if the users don't like the rewards, they can withdraw and their funds are not locked or lost.

Hence, planning to reject the escalation and leave the issue as it is.

Evert0x commented 3 months ago

Result: Invalid Unique

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: