c4-bot-2 commented 2 months ago

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);
    }
}