[M-01] Potential Division by Zero or Unintended Behavior Due to Close Asset Values in the `revoke` function

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/main/contracts/CollateralTracker.sol#L930-L986

Vulnerability details

Vulnerability Details

The revoke function in the smart contract has a medium severity issue related to the calculation of shares when revoking delegated shares. The issue arises from the calculation involving the potential division by zero or unintended behavior when the totalAssets() value is very close to or equal to the assets value being processed. Specifically, the problematic calculation is:

Math.mulDiv(assets, totalSupply - delegateeBalance, uint256(Math.max(1, int256(totalAssets()) - int256(assets))))

If totalAssets() is close to or equal to assets, the calculation could lead to an unintended division by a very small number, potentially resulting in a large and incorrect number of shares being minted or causing the transaction to revert.

Impact

The impact of this vulnerability includes:

• Unexpected Reverts: The transaction may unexpectedly revert due to a division by zero or near-zero value.
• Incorrect Share Calculation: If the calculation results in a large number due to a small denominator, it could lead to an incorrect number of shares being minted, potentially affecting the balance and integrity of the contract's operations.
• Collateral Mismanagement: This issue could lead to improper management of collateral, which could compromise the security and reliability of the financial operations of the smart contract.

Proof of Concept

Here is the original function with the potential vulnerability:

function revoke(
    address delegator,
    address delegatee,
    uint256 assets
) external onlyPanopticPool {
    uint256 shares = convertToShares(assets);

    // Get the delegatee balance and compare it later against the requested amount
    uint256 delegateeBalance = balanceOf[delegatee];

    if (shares > delegateeBalance) {
        // Transfer delegatee balance to delegator
        _transferFrom(delegatee, delegator, delegateeBalance);

        // Calculate the remaining shares to mint
        uint256 remainingShares = Math.mulDiv(
            assets,
            totalSupply - delegateeBalance,
            uint256(Math.max(1, int256(totalAssets()) - int256(assets)))
        );

        // Subtract delegatee balance from remaining shares since it was already transferred
        _mint(delegator, remainingShares - delegateeBalance);
    } else {
        // Transfer shares back if requested amount is less than delegatee balance
        _transferFrom(delegatee, delegator, shares);
    }
}

Tools Used

• Manual Review

Recommended Mitigation Steps

To mitigate the issue, ensure that the calculation handles edge cases properly and avoids division by zero or unintended behavior. Here is the revised function:

function revoke(
    address delegator,
    address delegatee,
    uint256 assets
) external onlyPanopticPool {
    uint256 shares = convertToShares(assets);

    // Get the delegatee balance and compare it later against the requested amount
    uint256 delegateeBalance = balanceOf[delegatee];

    // Check for the total supply and delegatee balance edge case
    require(totalSupply > delegateeBalance, "Total supply must be greater than delegatee balance");

    // Check for total assets and assets edge case
    require(totalAssets() > assets, "Total assets must be greater than assets");

    if (shares > delegateeBalance) {
        // Transfer delegatee balance to delegator
        _transferFrom(delegatee, delegator, delegateeBalance);

        // Calculate the remaining shares to mint
        uint256 remainingShares = Math.mulDiv(
            assets,
            totalSupply - delegateeBalance,
            uint256(Math.max(1, int256(totalAssets()) - int256(assets)))
        );

        // Subtract delegatee balance from remaining shares since it was already transferred
        _mint(delegator, remainingShares - delegateeBalance);
    } else {
        // Transfer shares back if requested amount is less than delegatee balance
        _transferFrom(delegatee, delegator, shares);
    }
}

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[Picodes]

How could it be 0 in a real example?