Bauchibred - Protocol inflates the amount of tokens received when depositing for some tokens

[sherlock-admin4]

Bauchibred

High

Protocol inflates the amount of tokens received when depositing for some tokens

Summary

Protocol inflates the amount of tokens received when depositing for some tokens

Vulnerability Detail

The readMe hints that the supported tokens are steth, weth & wsteth.

Now when depositing there is an instance where the amount of lp to process for the deposit is being processed, and in this instance, there is a need to route through all the tokens that are to be deposited and then calculate the depositValue which ends up being used to process the lp amounts, see https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/Vault.sol#L322-L338

    function deposit() {
..
        for (uint256 i = 0; i < tokens.length; i++) {
            uint256 priceX96 = priceOracle.priceX96(address(this), tokens[i]);
            totalValue += totalAmounts[i] == 0
                ? 0
                : FullMath.mulDivRoundingUp(totalAmounts[i], priceX96, Q96);
            if (ratiosX96[i] == 0) continue;
            uint256 amount = FullMath.mulDiv(ratioX96, ratiosX96[i], Q96);
            IERC20(tokens[i]).safeTransferFrom(
                msg.sender,
                address(this),
                amount
            );
            actualAmounts[i] = amount;
            depositValue += FullMath.mulDiv(amount, priceX96, Q96);
        }

        lpAmount = _processLpAmount(to, depositValue, totalValue, minLpAmount);
..
        }

Would be key to note that stETH is not a fee on transfer token, however it is a special token when it comes to it's transfer logic, navigating to lido's official docs we can see that there is a special section that talks about it's unique concept, i.e the "1-2 wei corner case", see https://docs.lido.fi/guides/lido-tokens-integration-guide/#1-2-wei-corner-case, quoting them:

stETH balance calculation includes integer division, and there is a common case when the whole stETH balance can't be transferred from the account while leaving the last 1-2 wei on the sender's account. The same thing can actually happen at any transfer or deposit transaction. In the future, when the stETH/share rate will be greater, the error can become a bit bigger. To avoid it, one can use transferShares to be precise.

More can be read on the "1-2 wei corner case" issue from here.

What this means in the context of Mellow is that, since in all deposits of the steth token the amount really transferred is being inflated, so we are to also expect an inflation in the amount of lpAmount that gets attached to this, with consequents deposits through the lifetime of the protocol, the disparity between the real amount of lp, and the value that's backing them is only going to get higher, would be key to note that even Lido has hinted that this round down could be larger in the future.

Impact

Accounting flaw in regards to the real deposited value, which not only means an inflation of the lpAmounts being processed, but also the fact that in the case of a bank run the last set of users might not be able to withdraw their assets considering with time the disparity between value deposited and lpAmount processed is only going to get accumulatively larger.

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/Vault.sol#L322-L338

Tool used

Manual Review

Recommendation

Since steth is supported, check the real amount of assets received when a transfer on this token is being processed.

[Bauchibred]

This should be a dup of #299, it talks about the 1-2 wei corner case and hints a different impact.

Report that hints something similar can be seen here: #115, which the judge has agreed to be considered valid mediums post escalation, based on his comments here & here.

[Al-Qa-qa]

Escalate.

On @Bauchibred Behalf.

[sherlock-admin3]

Escalate.

On @Bauchibred Behalf.

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.

[IlIlHunterlIlI]

vault doesn't support stETH but supports wstETH

[WangSecurity]

I believe the report is incorrect. The issue wouldn't result in inflated lp tokens, the issue results in the revert on deposit, so the impact is incorrect here, since it cannot be reached. Planning to reject the escalation and leave the issue as it is.

[Bauchibred]

I believe the report is incorrect. The issue wouldn't result in inflated lp tokens, the issue results in the revert on deposit, so the impact is incorrect here, since it cannot be reached. Planning to reject the escalation and leave the issue as it is.

What do you mean? How does it revert on deposit? Note that I hinted this to be a dup of #299, cause they both are talking about the 1-2 wei corner case problem and this was grounds for the duplications in the just concluded Sophon audit, see sherlock-audit/2024-05-sophon-judging#30, just like here, it's just the same bug case in different parts of the protocol.

Pending your further explanation, I assume your current confusion is from you considering the snippets to be similar with #299? In that case there is a conversion occurring and the amount is being attempted to be transferred twice (second instance is during the wrapping here). Here the assets are only transferred once, so no reverts occur, but the amount that's received is inflated when processing the lp amounts due to the 1-2 wei corner case (that could potentially be bigger & regardless this accumulates still with every transfer). So if #299 is validated, this could be duplicated to it, otherwise this could stand on its own and be validated still.

[IlIlHunterlIlI]

@Bauchibred sir, vaults doesn't support stETH as underlying

[WangSecurity]

@Bauchibred is correct, I've overlooked and made an incorrect comment. But as I understand based on the @IlIlHunterlIlI , which is correct, the problem described in this report, wouldn't happen with stETH in the deposit function, cause the protocol uses only the following tokens as underlying: As underlyingTokens only [weth, wsteth, reth, usdc, usdt] can be used https://discord.com/channels/812037309376495636/1252276613417013353/1254897219790831747

Hence, if the above assumption is correct, the decision is to reject the escalation and leave the issue as it is.

[Bauchibred]

As underlyingTokens only [weth, wsteth, reth, usdc, usdt] can be used https://discord.com/channels/812037309376495636/1252276613417013353/1254897219790831747

Hey @WangSecurity, thanks for providing this link, missed this info during the audit as I wasn't active on discord, so I assumed any whitelisted token could be used as underlying. With this, I believe @IlIlHunterlIlI is right that the vaults don't support stETH as underlying and as such this report should be invalidated and the escalation rejected.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Al-Qa-qa: rejected