CollateralTracker is not EIP4626 compliant: maxMint is calculated to be too large

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/CollateralTracker.sol#L444-L448

Vulnerability details

Impact

CollateralTracker is not EIP4626 compliant. Specifically, the maxMint is calculated to be too large, and users will fail minting the shares maxMint returns.

Bug Description

First, let's quote the EIP4626 doc https://eips.ethereum.org/EIPS/eip-4626:

maxMint

Maximum amount of shares that can be minted from the Vault for the receiver, through a mint call.

MUST return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

The maxMint value should never be overestimated, and user should always be able to mint the amount of assets maxMint returns.

However, this is not the case for CollateralTracker. For CollateralTracker, and the share to mint formula (by previewMint) is: assets = shares * DECIMALS * totalAssets / (totalSupply * (DECIMALS - COMMISSION_FEE)).

From which we can derive shares = assets * (totalSupply * (DECIMALS - COMMISSION_FEE)) / (totalAssets * DECIMALS), and given the maximum assets is uint104.max, the correct maximum shares (maxMint) should be convertToShares(type(uint104).max * (DECIMALS - COMMISSION_FEE)) / DECIMALS which is smaller than the current maxMint().

    /// @notice returns The maximum deposit amount.
    /// @return maxAssets The maximum amount of assets that can be deposited.
    function maxDeposit(address) external pure returns (uint256 maxAssets) {
        return type(uint104).max;
    }

    /// @notice Returns the maximum shares received for a deposit.
    /// @return maxShares The maximum amount of shares that can be minted.
    function maxMint(address) external view returns (uint256 maxShares) {
        unchecked {
            return (convertToShares(type(uint104).max) * DECIMALS) / (DECIMALS + COMMISSION_FEE);
        }
    }

    /// @notice Returns the amount of assets that would be deposited to mint a given amount of shares.
    /// @param shares The amount of shares to be minted.
    /// @return assets The amount of assets that would be deposited.
    function previewMint(uint256 shares) public view returns (uint256 assets) {
        // round up depositing assets to avoid protocol loss
        // This prevents minting of shares where the assets provided is rounded down to zero
        // compute the MEV tax, which is equal to a single payment of the commissionRate on the FINAL (post mev-tax) assets paid
        // finalAssets - convertedAssets = commissionRate * finalAssets
        // finalAssets - commissionRate * finalAssets = convertedAssets
        // finalAssets * (1 - commissionRate) = convertedAssets
        // finalAssets = convertedAssets / (1 - commissionRate)
        unchecked {
            assets = Math.mulDivRoundingUp(
                shares * DECIMALS,
                totalAssets(),
                totalSupply * (DECIMALS - COMMISSION_FEE)
            );
        }
    }

    function mint(uint256 shares, address receiver) external returns (uint256 assets) {
        assets = previewMint(shares);

        if (assets > type(uint104).max) revert Errors.DepositTooLarge();
        ...
    }

Proof of Concept

Add the following test code in CollateralTracker.t.sol. See that we try to mint maxMint() even with a 1e18 buffer, but it still fails.

    function test_maxMintDepositTooLarge() public {
        uint seed = 1;
        // initalize world state
        _initWorld(seed);

        // Invoke all interactions with the Collateral Tracker from user Bob
        vm.startPrank(Bob);

        // give Bob the max amount of tokens
        _grantTokens(Bob);

        // approve collateral tracker to move tokens on the msg.senders behalf
        IERC20Partial(token0).approve(address(collateralToken0), type(uint256).max);

        // change the share price a little so we know it's checking the assets
        collateralToken0.deposit(2 ** 64, Bob);

        IERC20Partial(token0).transfer(address(panopticPool), 2 ** 64);

        // We use `maxMint` as shares, and even give it a 1e18 buffer.
        uint shares = collateralToken0.maxMint(Bob) - 1e18;

        // However, the mint still fails due to DepositTooLarge.
        vm.expectRevert(Errors.DepositTooLarge.selector);
        collateralToken0.mint(shares, Bob);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Use convertToShares(type(uint104).max * (DECIMALS - COMMISSION_FEE)) / DECIMALS for maxMint function.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #553

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[dyedm1]

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

This is a good point and we will fix, but not sure it fulfills Medium severity given the lack of impact (& CollateralTracker is not on the compliance checklist).

The only impact of this is that if, for some reason, users attempt to mint with the result of maxMint (which is an unrealistic amount of tokens for most, if not all actively traded tokens), their transaction will revert (in which case they could just mint slightly less according to the "real" maxMint).

Even if maxMint was correct as to spec, their transaction would similarly revert if the share price changed before their transaction was included.

[stalinMacias]

Hello Judge, in case this remains as medium, I'd like to ask this finding from my QA report to be marked as a duplicate of this report.

In that finding, I explained the same issue reported in this report.

[Picodes]

As the contract is not in the compliance checklist, the argument for med "broken functionality", the functionality being the compliance to the EIP doesn't hold, so QA is more appropriate.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as not selected for report