c4-bot-1 commented 4 months ago

Lines of code

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

Vulnerability details

Details

Users can make deposits of Collateral tokens into the pool through the CollateralTracker.deposit and receive shares for their deposits. These shares can later be redeemed for their initial deposits. When deposits are made, the s_poolAssets is incremented with the assets deposited while the totalSupply is incremented with the shares minted to the user.

 function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
        if (assets > type(uint104).max) revert Errors.DepositTooLarge(); 

        shares = previewDeposit(assets);

        // transfer assets (underlying token funds) from the user/the LP to the PanopticPool
        // in return for the shares to be minted
        SafeTransferLib.safeTransferFrom(
            s_underlyingToken,
            msg.sender,
            address(s_panopticPool),
            assets
        );

        _mint(receiver, shares);

        // update tracked asset balance
       @> unchecked {
            s_poolAssets += uint128(assets);
        } 
        // 340282366920938463463374607431768211455
        //audit-note: 16 mil deposits (16777216) would need to be completed to make s_poolAssets overflow on this last deposit

        emit Deposit(msg.sender, receiver, assets, shares);
    }

The reverse happens when the shares are redeemed or withdrawn. s_poolAssets is incremented in an unchecked keyword allowing overflow. Since s_poolAssets is a uint128, and maxDeposit is uint104.max, it is highly possible to get s_poolAssets to uint128.max. Only 16 million max deposits are required to get this value. In a very active pool, getting to this value is a relatively small feat.

Impact

It is expected that users should be able to always redeem their shares for the same amount of asset initially deposited no matter the conditions in the pool. What happens when s_poolAssets gets to uint128.max, and a user deposits enough to cause an overflow (This can be as little as 100 assets deposit), that user would never get their assets back and their assets would be lost forever. Infact, all other users of the pool would also get seriously impacted as well. This breaks core protocol functionality. However, I believe the impact is a medium due to the edge case that s_poolAssets would eventually get to max value especially during periods of high deposits and low redeem or withdrawal. But still, this is still very likely to happen.

Proof of Concept

Insert the following into CollateralTracker.t.sol I have mocked the s_poolAssets to be close to type(uint128).max and also totalSupply in CollateralTracker has been modified close to type(uint128).max to manually balance the accounting.

function testAuditAssetsOverflow() 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);

        uint104 assets = 1000;

        // approve collateral tracker to move tokens on the msg.senders behalf
        IERC20Partial(token0).approve(address(collateralToken0), 1e30);
        IERC20Partial(token1).approve(address(collateralToken1), 1e30);

        // set pool assets to close to uint128.max
        // This allows Bob's deposit of 1000 assets to overflow s_poolAssets to 0
        // Note that no matter the deposit as long as overflow happens, Bob would lose assets
        collateralToken0.setPoolAssets(type(uint128).max - 999);
        collateralToken1.setPoolAssets(type(uint128).max - 999);

        // the amount of shares that can be minted
        // supply == 0 ? assets : FullMath.mulDiv(assets, supply, totalAssets());
        uint256 sharesToken0 = FullMath.mulDiv(
            uint256(assets) * 9_990,
            collateralToken0.totalSupply(),
            collateralToken0.totalAssets() * 10_000
        );
        uint256 sharesToken1 = FullMath.mulDiv(
            uint256(assets) * 9_990,
            collateralToken1.totalSupply(),
            collateralToken1.totalAssets() * 10_000
        );

        // equal deposits for both collateral token pairs for testing purposes
        uint256 returnedShares0 = collateralToken0.deposit(assets, Bob);
        uint256 returnedShares1 = collateralToken1.deposit(assets, Bob);

        // check shares were calculated correctly
        assertEq(sharesToken0, returnedShares0);
        assertEq(sharesToken1, returnedShares1);
        // check if receiver got the shares
        assertEq(sharesToken0, collateralToken0.balanceOf(Bob));
        assertEq(sharesToken1, collateralToken1.balanceOf(Bob));

        // check the new s_poolAssets and totalSupply
        collateralToken0.totalAssets();
        collateralToken0.totalSupply();
        collateralToken1.totalAssets();
        collateralToken1.totalSupply();

        // We can see that Bob's redeem for collateralToken0 is 0.
        // So Bob has lost his initial assets
        uint256 redeemAssets0 = collateralToken0.previewRedeem(collateralToken0.balanceOf(Bob));
        uint256 redeemAssets1 = collateralToken0.previewRedeem(collateralToken1.balanceOf(Bob));
        assertEq(redeemAssets0, 0);
        // Redeem full shares and nothing gets redeemed
        collateralToken0.redeem(collateralToken0.balanceOf(Bob), Bob, Bob);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Further deposits can be prevented once the s_poolAssets is at uint128.max. This can be checked prior to every deposit in CollateralTracker.previewDeposit

function previewDeposit(uint256 assets) public view returns (uint256 shares) {
+       if(s_poolAssets + assets > type(uint128).max) revert;
    unchecked {
        // a*b/c
        shares = Math.mulDiv(
            assets * (DECIMALS - COMMISSION_FEE),
            totalSupply,
            totalAssets() * DECIMALS
        );
    }
}

This should effectively mitigate this issue.

Additionally, the use of unchecked when incrementing s_poolAsset can be reconsidered. If this is remove, when user attempts to stake amount which would exceed max, solidity would naturally revert preventing the deposit.

Assessed type

Under/Overflow

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Insufficient proof

Picodes commented 4 months ago

What about this line https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L418 ?

UnknownBlackman commented 4 months ago

Hello, if you test the poc you would see that it is just as I said. Please kindly review this issue as it is not "unsatisfactory" and not eligible but supposed to be eligible. Thank you for your assistance

UnknownBlackman commented 4 months ago

What about this line https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L418 ?

Sorry what does this mean? Users can make deposits of up to type(uint104).max so it is just as I said. In periods of high deposits and low withdrawals with a very active pool when s_poolAssets overflow this would break core protocol invariant as users lose deposited assets.

Picodes commented 3 months ago

@UnknownBlackman my bad, please disregard my previous comment. I still believe this issue is out of scope considering the readme:

"Very large quantities of tokens are not supported. It should be assumed that for any given pool, the cumulative amount of tokens that enter the system (associated with that pool, through adding liquidity, collection, etc.) will not exceed 2^127 - 1. Note that this is only a per-pool assumption, so if it is broken on one pool it should not affect the operation of any other pools, only the pool in question."

UnknownBlackman commented 3 months ago

@Picodes Thank you for taking your time to review this issue. I appreciate. However, I still believe this issue to be valid.

The statement that the total accumulated assets in a pool would not exceed 2^127-1 does not have a check within the code implementation itself. The team seems to be making assumptions on this without implementing anything to handle this edge case. The code should always supercede assumptions, and since there is no check that deposited assets cannot exceed max uint128, then it should be treated as a possibility. This is a pool where possibly even millions of users would participate in, each asset deposit is denominated as 1e18. And at max deposit of 2^104-1 that requires just 16 million deposits to get to that value. The protocol team doesn't have a 100% guarantee that that would never been achieved as they cannot control the market once it gets into production.
A very critical invariant with any defi protocol such as this is that users should never lose that deposited assets at any point and such breaks should be treated as a vulnerability. This report demonstrates that users do in fact lose their deposited assets. Infact, not just Bob but every single depositor in that pool would lose their assets or get skewed returns when redeeming.
The case affects just one pool but what if that is the most active pool in the protocol where majority of users have deposited their assets. Even this happening in one pool can strongly affect behaviour towards other active pools and even the entire protocol. We can see that a case like that would strongly affect the protocol, users and even sentiments towards the protocol itself. Tldr: I believe the protocol to be making unsafe assumptions about how their pools would operate which they do not have 100% certainty for. Users can get seriously impacted and this happening would damage the business of the protocol itself.

Thank you and I hope you reconsider