xiaoming90 - previewRedeem and redeem functions deviate from the ERC4626 specification

[sherlock-admin2]

xiaoming90

medium

previewRedeem and redeem functions deviate from the ERC4626 specification

Summary

The previewRedeem and redeem functions deviate from the ERC4626 specification. As a result, the caller (internal or external) of the previewRedeem function might receive incorrect information, leading to the wrong decision being executed.

Vulnerability Detail

Important
The contest page explicitly mentioned that the LMPVault must conform with the ERC4626. Thus, issues related to EIP compliance should be considered valid in the context of this audit.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?

src/vault/LMPVault.sol should be 4626 compatible

Let the value returned by previewRedeem function be $asset_{preview}$ and the actual number of assets obtained from calling the redeem function be $asset_{actual}$.

The following specification of previewRedeem function is taken from ERC4626 specification:

Allows an on-chain or off-chain user to simulate the effects of their redeemption at the current block, given current on-chain conditions.

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the same transaction.

It mentioned that the redeem should return the same or more assets as previewRedeem if called in the same transaction, which means that it must always be $asset_{preview} \le asset_{actual}$.

However, it is possible that the redeem function might return fewer assets than the number of assets previewed by the previewRedeem ($asset_{preview} > asset_{actual}$), thus it does not conform to the specification.

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L422

File: LMPVault.sol
422:     function redeem(
423:         uint256 shares,
424:         address receiver,
425:         address owner
426:     ) public virtual override nonReentrant noNavDecrease ensureNoNavOps returns (uint256 assets) {
427:         uint256 maxShares = maxRedeem(owner);
428:         if (shares > maxShares) {
429:             revert ERC4626ExceededMaxRedeem(owner, shares, maxShares);
430:         }
431:         uint256 possibleAssets = previewRedeem(shares); // @audit-info  round down, which is correct because user won't get too many
432: 
433:         assets = _withdraw(possibleAssets, shares, receiver, owner);
434:     }

Note that the previewRedeem function performs its computation based on the cached totalDebt and totalIdle, which might not have been updated to reflect the actual on-chain market condition. Thus, these cached values might be higher than expected.

Assume that totalIdle is zero and all WETH has been invested in the destination vaults. Thus, totalAssetsToPull will be set to $asset_{preview}$.

If a DV is making a loss, users could only burn an amount proportional to their ownership of this vault. The code will go through all the DVs in the withdrawal queue (withdrawalQueueLength) in an attempt to withdraw as many assets as possible. However, it is possible that the totalAssetsPulled to be less than $asset_{preview}$.

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L448

File: LMPVault.sol
448:     function _withdraw(
449:         uint256 assets,
450:         uint256 shares,
451:         address receiver,
452:         address owner
453:     ) internal virtual returns (uint256) {
454:         uint256 idle = totalIdle;
455:         WithdrawInfo memory info = WithdrawInfo({
456:             currentIdle: idle,
457:             assetsFromIdle: assets >= idle ? idle : assets,
458:             totalAssetsToPull: assets - (assets >= idle ? idle : assets),
459:             totalAssetsPulled: 0,
460:             idleIncrease: 0,
461:             debtDecrease: 0
462:         });
463: 
464:         // If not enough funds in idle, then pull what we need from destinations
465:         if (info.totalAssetsToPull > 0) {
466:             uint256 totalVaultShares = totalSupply();
467: 
468:             // Using pre-set withdrawalQueue for withdrawal order to help minimize user gas
469:             uint256 withdrawalQueueLength = withdrawalQueue.length;
470:             for (uint256 i = 0; i < withdrawalQueueLength; ++i) {
471:                 IDestinationVault destVault = IDestinationVault(withdrawalQueue[i]);
472:                 (uint256 sharesToBurn, uint256 totalDebtBurn) = _calcUserWithdrawSharesToBurn(
473:                     destVault,
474:                     shares,
475:                     info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled),
476:                     totalVaultShares
477:                 );
..SNIP..
508:         // At this point should have all the funds we need sitting in in the vault
509:         uint256 returnedAssets = info.assetsFromIdle + info.totalAssetsPulled;

Impact

It was understood from the protocol team that they anticipate external parties to integrate directly with the LMPVault (e.g., vault shares as collateral). Thus, the LMPVault must be ERC4626 compliance. Otherwise, the caller (internal or external) of the previewRedeem function might receive incorrect information, leading to the wrong action being executed.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L422

Tool used

Manual Review

Recommendation

Ensure that $asset_{preview} \le asset_{actual}$.

Alternatively, document that the previewRedeem and redeem functions deviate from the ERC4626 specification in the comments and/or documentation.

[sherlock-admin2]

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

Trumpero commented:

low, not following ERC4626 won't incur risk for the users and protocol
To make sure the assetPreview <= assetActual, users (integrated protocol) should use router to redeem

[xiaoming9090]

Escalate

I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. The router is not applicable in the context of this issue, as ERC4626 is strictly applied to the LMPVault only.

Per the judging docs, the issue will be considered as valid if there is external integrations.

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, is considered informational

Also, the contest page explicitly mentioned that the LMPVault must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid in the context of this audit.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?

src/vault/LMPVault.sol should be 4626 compatible

In this case, non-conforming to ERC4626 is a valid Medium.

[sherlock-admin2]

Escalate

I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. The router is not applicable in the context of this issue, as ERC4626 is strictly applied to the LMPVault only.

Per the judging docs, the issue will be considered as valid if there is external integrations.

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, is considered informational

Also, the contest page explicitly mentioned that the LMPVault must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid in the context of this audit.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?

src/vault/LMPVault.sol should be 4626 compatible

In this case, non-conforming to ERC4626 is a valid Medium.

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.

[midori-fuse]

Escalate

If this issue ends up being valid post-escalation, then #452 #441 #319 #288 #202 should be dupes of this.

[sherlock-admin2]

Escalate

If this issue ends up being valid post-escalation, then #452 #441 #319 #288 #202 should be dupes of this.

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.

[JeffCX]

I thought the EIP complicance issue is valid low in sherlock

[JeffCX]

https://docs.sherlock.xyz/audits/judging/judging

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational

[0xbtk]

If #577 is considered a medium, then #412 should be a medium too, because as per of the EIP-4626:

It is considered most secure to favor the Vault itself during calculations over its users.

[Kalyan-Singh]

#656 shows how deposit function reverts under certain conditions due to maxDeposit not being eip compliant, I think that will be a genuine problem for external integrators. I think this escalations result should also be reflected there.

[xiaoming9090]

I thought the EIP complicance issue is valid low in sherlock

For this contest, it was explicitly mentioned in the contest page that the LMPVault must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid Medium in the context of this audit.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?

src/vault/LMPVault.sol should be 4626 compatible

[xiaoming9090]

https://docs.sherlock.xyz/audits/judging/judging

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational

I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. Thus, there are external integrations.

[0xSurena]

I thought the EIP complicance issue is valid low in sherlock

For this contest, it was explicitly mentioned in the contest page that the LMPVault must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid Medium in the context of this audit.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible

Exactly, it was explicitly mentioned in the contest page that code/contract expected / should to comply with 4626.

@JeffCX

https://docs.sherlock.xyz/audits/judging/judging

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational

In case of conflict between information in the Contest README vs Sherlock rules, the README overrides Sherlock rules.

[Trumpero]

I believe this issue is a low/info issue evidently. Judging docs of Sherlock clearly stated that:

EIP compliance with no integrations: If the protocol does not have external integrations, then issues related to the code not fully complying with the EIP it is implementing, and there are no adverse effects of this, it is considered informational.

Therefore, it should be an informational issue.

The issue must cause a loss of funds (even if unlikely) to be considered as medium

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

Furthermore, according to Sherlock's judging docs, the external integrations in the future are not considered valid issues:

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

[Evert0x]

Current opinion is to accept escalation and make issue medium because of the following judging rule.

In case of conflict between information in the README vs Sherlock rules, the README overrides Sherlock rules.
https://docs.sherlock.xyz/audits/judging/judging#iii.-some-standards-observed

[Trumpero]

I believe this issue doesn't meet the requirements of a medium issue since it doesn't cause any loss, which is an important requirement to be a valid issue in Sherlock. Even without considering the Sherlock docs about EIP compliance, it only has a low impact, so I don't think it is a valid medium. Historically, the potential risk from a view function has never been accepted as a medium in Sherlock. Additionally, there is no potential loss since users should use the router to redeem, which protects users from any loss by using the minimum redeem amount.

[midori-fuse]

Historically, the potential risk from a view function has never been accepted as a medium in Sherlock

Disputing this point. Evidence

[xiaoming9090]

The README explicitly stated that LMPVault.sol should be 4626 compatible. README overwrites the Sherlock rules. Thus, any 4626 incompatible in this contest would be classified as Medium.

[Evert0x]

The core question is, does the README also override the severity classifications? It states that "Sherlock rules for valid issues" are overwritten. But it's unclear if the severity definitions are included in this, especially because the medium/high definitions are stated above this rule.

The intention of this sentence is that the protocol can thrive in the context defined by the protocol team.

In this case, it's clear that the team states that the LMPVault.sol should exist in the context of complete ERC4626 compatibility. Making this issue valid.

Planning to make some changes to the Medium definition and Hierarchy of truth language so it will be clear for future contests.

[Evert0x]

Planning to accept escalation and duplicate with #452 #441 #319 #288 #202

[Evert0x]

Update: planning to add #412 #577 #656 #487 and categorize as a general "Failing to comply with ERC4626" issue family.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted
• midori-fuse: accepted

[Evert0x]

Will add #665 #465, #503 and #731 as duplicates as they point out an issue that will make the contract fail to comply with ERC4626.

Because _maxMint() has the possibility to return uint256.max it can break the ERC4626 specification of maxDeposit of "MUST NOT REVERT"

See #665 (comment)