maxMint() in CollateralTracker.sol is not compliant with EIP-4626

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The very similar issue was reported as Medium during the recent Code4rena contest: https://github.com/code-423n4/2024-03-pooltogether-findings/issues/335.
That contest reported that maxDeposit() might return a value greater than can be deposited, violating EIP-4626. The current issue is: maxMint() might return a value greater than can be minted, violating EIP-4626.

According to provided docs - CollateralTracker.sol is an ERC4626 vault where token liquidity from passive Panoptic Liquidity Providers (PLPs) and collateral for option positions are deposited.

However, the current implementation does not comply with EIP-4626. Since the compliance requirement was straightforwardly mentioned as a requirement in the docs - I've evaluated this issue as Medium.

This may cause unexpected behavior due to being non compliant with EIP-4626. Other protocols that integrate with contract may incorrectly assume that it's EIP-4626 compliant - especially that documentation states that it's ERC-4626. EIP-4626 purpose is to create a robust and consistent implementation patterns for vaults. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a vault and describes it as ERC-4626, it should fully conform to EIP-4626 standard.

Proof of Concept

According to EIP-4626, function maxMint returns maximum amount of shares that can be minted from the Vault for the receiver, through a mint call..

Moreover, according to EIP-4626:

mint

MUST revert if all of shares cannot be minted (due to deposit limit being reached, slippage, the user not approving enough underlying tokens to the Vault contract, etc).

Let's take a look at the current implementation of these functions.

File: CollateralTracker.sol

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

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

Function mint() verifies the assets amount against the hardcoded value type(uint104).max. It does not, however, utilize (call) maxMint() at all.
This is incorrect, because, according to EIP-4626, function maxMint() is responsible for returning the maximum amount of shares that can be minted from the Vault.

File: CollateralTracker.sol

    function maxMint(address) external view returns (uint256 maxShares) {
        unchecked {
            return (convertToShares(type(uint104).max) * DECIMALS) / (DECIMALS + COMMISSION_FEE);
        }
    }

As we clearly see in the implementation of maxMint(), it might return different value than type(uint104).max.

To demonstrated that indeed those values might be different, we've prepared a simple PoC which can be used in Remix-IDE.
The code had been slightly modified, to demonstrate the current settings in which maxMint() will return a value greater than can be minted (e.g. COMMISSION_FEE has been set to 50, totalAssets() is set to 1000000 and so on).

contract PoC {
using Math for uint256;

error DepositTooLarge();

uint256 DECIMALS = 10000;
uint totalSupply = 10 ** 6;
uint COMMISSION_FEE = 50;
uint public totalAssets = 10 ** 6;

  function convertToShares(uint256 assets) public view returns (uint256 shares) {
        return Math.mulDiv(assets, totalSupply, totalAssets);
    }

    function convertToAssets(uint256 shares) public view returns (uint256 assets) {
        return Math.mulDiv(shares, totalAssets, totalSupply);
    }

      function maxMint(address) external view returns (uint256 maxShares) {
        unchecked {
            return (convertToShares(type(uint104).max) * DECIMALS) / (DECIMALS + COMMISSION_FEE);
        }
    }

    function previewMint(uint256 shares) public view returns (uint256 assets) {
 
        unchecked {
            assets = Math.mulDivRoundingUp(
                shares * DECIMALS,
                totalAssets,
                totalSupply * (DECIMALS - COMMISSION_FEE)
            );
        }
    }

    function mint(uint256 shares) external returns (uint256 assets) {
        assets = previewMint(shares);
        console.log(assets);
        if (assets > type(uint104).max) revert DepositTooLarge();

    }

    
} 

After deploying above contract, we can see, that function maxMint() returns 20181502093185741715370399289567.

This indicated, that we should be able to call mint() with any value equal or lower than 20181502093185741715370399289567.

Let's try to call mint(20181502093185741715370300000000). 20181502093185741715370300000000 is significantly lower than 20181502093185741715370399289567, thus it should be possible to call mint() with that parameter. However, calling mint(20181502093185741715370300000000) will revert. This means that contract violates EIP-4626 standard - maxMint() returned a value greater than can be minted.

Let's examine what really happened.

1. maxMint() returns 20181502093185741715370399289567. Thus it should be possible to mint max 20181502093185741715370399289567 shares.
2. We will try to mint 20181502093185741715370300000000 shares (20181502093185741715370300000000 < 20181502093185741715370399289567) thus function should not revert.
3. Calling mint(20181502093185741715370300000000) however reverts with DepositTooLarge() error.

This simply demonstrated that maxMint() might return a value greater than can be minted, violating EIP-4626.

Tools Used

Manual code review

Recommended Mitigation Steps

Utilize maxMint() directly in the mint() function. E.g., please take a look at an example how it's done in different functions:

function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) external returns (uint256 shares) {
        if (assets > maxWithdraw(owner)) revert Errors.ExceedsMaximumRedemption();

Function withdraw() straightforwardly checks if assets > maxWithdraw(owner).

 function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) external returns (uint256 assets) {
        if (shares > maxRedeem(owner)) revert Errors.ExceedsMaximumRedemption();

Function redeem() straightforwardly checks if shares > maxRedeem(owner).

The same should be done for function mint():

function mint(uint256 shares, address receiver) external returns (uint256 assets) {
	if (shares > maxMint(receiver)) revert Errors.ExceedsMaximumMint();

Assessed type

ERC4626

[c4-judge]

Picodes marked the issue as duplicate of #553

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)