Kalogerone - The SuperPool vault is not strictly ERC4626 compliant as it should be

[sherlock-admin2]

Kalogerone

Medium

The SuperPool vault is not strictly ERC4626 compliant as it should be

Summary

The contest README file clearly states that:

Q: Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification?

SuperPool.sol is strictly ERC4626 compliant

No deviations from the specification mentioned. The SuperPool.sol contract is not strictly ERC4626 compliant according to the EIP docs.

Root Cause

The EIP docs for the convertToShares and convertToAssets functions state:

MUST NOT be inclusive of any fees that are charged against assets in the Vault.

and later also state:

The convertTo functions serve as rough estimates that do not account for operation specific details like withdrawal fees, etc. They were included for frontends and applications that need an average value of shares or assets, not an exact value possibly including slippage or other fees. For applications that need an exact value that attempts to account for fees and slippage we have included a corresponding preview function to match each mutable function. These functions must not account for deposit or withdrawal limits, to ensure they are easily composable, the max functions are provided for that purpose.

However, SuperPool's convertToShares and convertToAssets also calculate and include any new fees accrued.

    /// @notice Converts an asset amount to a share amount, as defined by ERC4626
    /// @param assets The amount of assets
    /// @return shares The equivalent amount of shares
    function convertToShares(uint256 assets) public view virtual returns (uint256 shares) {
 @>     (uint256 feeShares, uint256 newTotalAssets) = simulateAccrue();
 @>     return _convertToShares(assets, newTotalAssets, totalSupply() + feeShares, Math.Rounding.Down);
    }

    /// @notice Converts a share amount to an asset amount, as defined by ERC4626
    /// @param shares The amount of shares
    /// @return assets The equivalent amount of assets
    function convertToAssets(uint256 shares) public view virtual returns (uint256 assets) {
@>      (uint256 feeShares, uint256 newTotalAssets) = simulateAccrue();
@>      return _convertToAssets(shares, newTotalAssets, totalSupply() + feeShares, Math.Rounding.Down);
    }

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

The SuperPool is not strictly EIP-4626 compliant as the README file states it should be.

PoC

No response

Mitigation

Don't calculate any new fees accrued in the external convertTo functions:

    function convertToShares(uint256 assets) public view virtual returns (uint256 shares) {
-       (uint256 feeShares, uint256 newTotalAssets) = simulateAccrue();
-       return _convertToShares(assets, newTotalAssets, totalSupply() + feeShares, Math.Rounding.Down);
+       return _convertToShares(assets, totalAssets(), totalSupply(), Math.Rounding.Down);
    }

    function convertToAssets(uint256 shares) public view virtual returns (uint256 assets) {
-       (uint256 feeShares, uint256 newTotalAssets) = simulateAccrue();
-       return _convertToAssets(shares, newTotalAssets, totalSupply() + feeShares, Math.Rounding.Down);
+       return _convertToAssets(shares, totalAssets(), totalSupply(), Math.Rounding.Down);
    }

[neko-nyaa]

Escalate

Within this issue family, there are several issues having to do with the inclusion of fees, while others deal with the pool's (lack of) liquidity. Some of the issues has to be moved over to #129 which deals with liquidity, or the other family has to be duped against this one.

[sherlock-admin3]

Escalate

Within this issue family, there are several issues having to do with the inclusion of fees, while others deal with the pool's (lack of) liquidity. Some of the issues has to be moved over to #129 which deals with liquidity, or the other family has to be duped against this one.

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.

[10xhash]

Issues #246 and #129, also mentions not following ERC4626 spec. Either every breaking of the spec should be grouped under one single issue, or each breaking should be considered as a different issue for consistency. But this is not followed here as highlighted in the above comment

[Nihavent]

Escalate

The subset of this family relating to fees taken should be invalid.

convertToShares & convertToAssets must simulateAccrue to update state before performing a conversion. The only fees included are calculated on prior interest earned which has nothing to do with the current conversion. They're required to be included to ensure the total shares represents an accurate state of the contract.

Removing the fees since the last state update will remove some fees but not all previous fees. Any change made to remove the fees since last update would make these functions useless to end users or external protocols as they would perform calculations on invalid states.

Therefore the 'fix' here would introduce a bug. Or you could completely redesign the contract to separate fee shares and implement useless versions of the 'convertTo...' functions for the sake of absolute ERC4626 compliance, neither of which are helpful to the protocol.

[sherlock-admin3]

Escalate

The subset of this family relating to fees taken should be invalid.

convertToShares & convertToAssets must simulateAccrue to update state before performing a conversion. The only fees included are calculated on prior interest earned which has nothing to do with the current conversion. They're required to be included to ensure the total shares represents an accurate state of the contract.

Removing the fees since the last state update will remove some fees but not all previous fees. Any change made to remove the fees since last update would make these functions useless to end users or external protocols as they would perform calculations on invalid states.

Therefore the 'fix' here would introduce a bug. Or you could completely redesign the contract to separate fee shares and implement useless versions of the 'convertTo...' functions for the sake of absolute ERC4626 compliance, neither of which are helpful to the protocol.

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.

[jsmi0703]

@10xhash

Either every breaking of the spec should be grouped under one single issue, or each breaking should be considered as a different issue for consistency.

If you think so, why did you submit #567 and #568 respectively while not submit them in one report. I think that the reports which have different root cause in the code base can't be grouped into a single family.

[cvetanovv]

I agree with the escalation of @neko-nyaa all issues related to ERC4626 compliant to be duplicated together. This will be the main issue.

Planning to accept the escalation and duplicate this issue with #129, #500, #465 and #246 and its duplicates.

[jsmi0703]

I disagree the escalation. Sherlock has rule for duplication.

1. Identify the root cause

So, we have to group only reports which have the same root cause. Moreover,

Root cause groupings

If the following issues appear in multiple places, even in different contracts. In that case, they may be considered to have the same root cause.

1. Issues with the same logic mistake.
   Example: uint256 is cast to uint128 unsafely.

2. Issues with the same conceptual mistake.
   Example: different untrusted external admins can steal funds.

3. Issues in the category
   Slippage protection
   Reentrancy
   Access control
   Front-run / sandwich ( issue A that identifies a front-run and issue B that identifies a sandwich can be duplicated )

The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they may be treated separately.

There are no item for ERC4626 at all. Therefore, ERC4626 compliant can't be the root cause for grouping. "ERC4626 compliant" is impact not root cause.
#110, #129, #246 should not be grouped in one family because they have different root causes.

[debugging3]

I agree on the above comment.As I know, they are going to be grouped by conceptual mistake rule. But the rule depends on the context. Otherwise, auditors will not submit all the vulnerabilities they found.

For instance, assume that an auditor found two issues: first one for convertToShares() and second one for maxWithdraw(). If they should be grouped in one issue, the auditor has no need of submitting the second issue and he will not submit it. Then how can the protocol team aware of the vulnerability inside the maxWithdraw() and fix it?

The protocol team already know that the pool should comply with ERC4626. The things they don't know are detailed vulnerabilities, not the broken "ERC4626 compliant" itself. So, I believe that the issues should be categorized into several issues according to their root cause in the code base.

[cvetanovv]

My decision is to group all ERC4626 related issues together.

I duplicate them by the rule of the same conceptual mistake.

I will give an example of how all ERC4626-related issues can be duplicated by showing an example from a previous contest. Here, all Weird Tokens are duplicated together by the same conceptual mistake rule: sherlock-audit/2024-06-magicsea-judging#545 (comment)

Planning to reject both escalations but duplicate this issue with #129, #500, #465, and #246(and its duplicates).

[Nihavent]

Hi I'm wondering if this #110 (comment) escalation was considered?

[cvetanovv]

All will remain duplicated together.

The Readme states that the protocol wants to strictly follow the ERC4626 standard. And the Watsons have correctly pointed out what is not followed. The protocol is not required to fix their issues.

My decision is to reject both escalations but duplicate this issue with #129, #500, #465, and #246(and its duplicates).

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• neko-nyaa: rejected
• Nihavent: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
sentimentxyz/protocol-v2#342