Lines of code

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

Vulnerability details

Vulnerability Description

maxRedeem and maxWithdraw functions check if the user has open positions via s_panopticPool.numberOfPositions(owner) == 0. However, a malicious user could manipulate their numberOfPositions (e.g. by double-counting a tokenId) to make it appear as zero even with open positions. This would let them incorrectly withdraw funds.

    function maxRedeem(address owner) public view returns (uint256 maxShares) {
        uint256 available = convertToShares(s_poolAssets);
        uint256 balance = balanceOf[owner];
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

    function maxWithdraw(address owner) public view returns (uint256 maxAssets) {
        // We can only use the standard 4626 withdraw function if the user has no open positions
        // For the sake of simplicity assets can only be withdrawn through the redeem function
        uint256 available = s_poolAssets;
        uint256 balance = convertToAssets(balanceOf[owner]);
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

They check if the user has any open positions by calling the numberOfPositions function. If the user has no open positions, the functions return the minimum of the available shares/assets and the user's balance. Otherwise, they return zero.

Impact

Reliance on the numberOfPositions to determine if a user has open positions. A malicious user can manipulate their numberOfPositions value to make it appear as zero, even when they have open positions.

        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;

Malicious users can withdraw funds from the CollateralTracker contract even when they have open positions.

Proof of Concept

The root cause lies in the reliance on the numberOfPositions function from the PanopticPool contract to determine if a user has open positions. The maxRedeem and maxWithdraw functions in the CollateralTracker contract use the result of numberOfPositions to decide whether to allow a user to redeem shares or withdraw assets.

   function maxRedeem(address owner) public view returns (uint256 maxShares) {
       uint256 available = convertToShares(s_poolAssets);
       uint256 balance = balanceOf[owner];
       return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
   }

    function maxWithdraw(address owner) public view returns (uint256 maxAssets) {
        // We can only use the standard 4626 withdraw function if the user has no open positions
        // For the sake of simplicity assets can only be withdrawn through the redeem function
        uint256 available = s_poolAssets;
        uint256 balance = convertToAssets(balanceOf[owner]);
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

If the numberOfPositions function in the PanopticPool contract is manipulated by a malicious user to return zero even when they have open positions, it allows them to bypass the intended restriction and incorrectly redeem shares or withdraw assets.

Let's simulates an attack scenario

   pragma solidity ^0.8.0;

   import "./CollateralTracker.sol";
   import "./PanopticPool.sol";

   contract MaliciousUser {
       CollateralTracker public collateralTracker;
       PanopticPool public panopticPool;

       constructor(CollateralTracker _collateralTracker, PanopticPool _panopticPool) {
           collateralTracker = _collateralTracker;
           panopticPool = _panopticPool;
       }

       function attackWithdraw() external {
           // Assume the malicious user has open positions
           // ...

           // Manipulate the numberOfPositions value to return zero
           // (This is a simplified example, the actual manipulation may vary)
           panopticPool.manipulateNumberOfPositions(address(this), 0);

           // Withdraw assets from the CollateralTracker contract
           uint256 assetsToWithdraw = collateralTracker.maxWithdraw(address(this));
           collateralTracker.redeem(assetsToWithdraw, address(this), address(this));
       }
   }

The MaliciousUser contract represents a malicious user who has open positions in the PanopticPool contract. The attackWithdraw function is called to perform the attack.

The malicious user first manipulates the numberOfPositions value associated with their address to return zero, even though they have open positions. This manipulation is achieved through a hypothetical manipulateNumberOfPositions function in the PanopticPool contract (the actual manipulation may vary depending on the implementation).

After manipulating the numberOfPositions value, the malicious user calls the maxWithdraw function in the CollateralTracker contract to determine the maximum amount of assets they can withdraw. Since numberOfPositions returns zero, maxWithdraw incorrectly allows the withdrawal.

Finally, the malicious user calls the redeem function in the CollateralTracker contract to withdraw the assets, effectively bypassing the restriction that should have prevented the withdrawal due to their open positions.

Tools Used

Manual audit, VS Code

Recommended Mitigation

Ensure numberOfPositions cannot be manipulated and consider additional tracking of a user's open positions.

Introduce a mapping in the CollateralTracker contract that keeps track of the user's open positions. This mapping can be updated whenever a user mints or burns positions through the PanopticPool contract.

mapping(address => uint256) private s_userOpenPositions;

function mintCallback(address user, uint256 positionId) external {
    require(msg.sender == address(s_panopticPool), "Only PanopticPool can call");
    s_userOpenPositions[user] += 1;
}

function burnCallback(address user, uint256 positionId) external {
    require(msg.sender == address(s_panopticPool), "Only PanopticPool can call");
    require(s_userOpenPositions[user] > 0, "No open positions");
    s_userOpenPositions[user] -= 1;
}

function maxRedeem(address owner) public view returns (uint256 maxShares) {
    uint256 available = convertToShares(s_poolAssets);
    uint256 balance = balanceOf[owner];
    return s_userOpenPositions[owner] == 0 ? Math.min(available, balance) : 0;
}

function maxWithdraw(address owner) public view returns (uint256 maxAssets) {
    uint256 available = s_poolAssets;
    uint256 balance = convertToAssets(balanceOf[owner]);
    return s_userOpenPositions[owner] == 0 ? Math.min(available, balance) : 0;
}

In this mitigation, the CollateralTracker contract maintains its own s_userOpenPositions mapping to track the number of open positions for each user. The PanopticPool contract calls the mintCallback and burnCallback functions whenever a user mints or burns positions, updating the s_userOpenPositions mapping accordingly. The maxRedeem and maxWithdraw functions then use the s_userOpenPositions mapping instead of relying on the numberOfPositions function from the PanopticPool contract.

Assessed type

Invalid Validation

code-423n4 / 2024-04-panoptic-findings

`maxRedeem` and `maxWithdraw` can be manipulated to allow withdrawals with open positions #455