maxMint() violates EIP-4626

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

CollateralTracker.maxMint() is incorrect and violates EIP-4626.

Proof of Concept

In CollateralTracker.sol

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

whereas mint() is only limited by

assets = previewMint(shares);

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

where previewMint() is

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

This means that maxMint() should rather return type(uint104).max * totalSupply * (DECIMALS - COMMISSION_FEE) / (totalAssets() * DECIMALS).

There are several errors in the original maxMint(): the + in DECIMALS + COMMISSION_FEE, the inversion of DECIMALS / (DECIMALS - COMMISSION_FEE), and the use of convertToShares(). The latter causes a rounding error from a multiplication on a division.

CollateralTracker is an ERC4626 vault. Since DECIMALS / (DECIMALS + COMMISSION_FEE) > (DECIMALS - COMMISSION_FEE) / DECIMALS maxMint() returns too much. This can also be seen in the test case below which on the current code reverts due to DepositTooLarge(). This means that maxMint() violates "MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary)".

Paste the below test case into CollateralTracker.t.sol and run with forge test --match-test test_maxMint. This test should pass. It fails with the current code, but passes with the recommended fix below.

function test_maxMint() public {
    // initalize world state
    _initWorld(0);

    // 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(836459287459287647, Bob);

    uint256 maxMint = collateralToken0.maxMint(Bob);

    uint256 id = vm.snapshot();
    collateralToken0.mint(maxMint, Bob);

    vm.revertTo(id);
    vm.expectRevert(Errors.DepositTooLarge.selector);
    collateralToken0.mint(maxMint + 1, Bob);
}

Recommended Mitigation

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

Assessed type

ERC4626

[c4-judge]

Picodes marked the issue as primary issue

[dyedm1]

This is correct but EIP-4626 is not listed in the compliance requirements, so unsure whether Medium or Low severity would be most appropriate (the impact of this is very limited given that it is only an issue in the maxMint function, which is not used anywhere in the protocol or periphery).

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked issue #501 as primary and marked this issue as a duplicate of 501

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b