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

9 stars 4 forks source link

Incorrect force-exercising fee calculation #476

Closed c4-bot-10 closed 6 months ago

c4-bot-10 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

When force-exercising fee is calculated, the amount for loss in value in chunk is summed up so that it can be compensated for the option buyer, but by a mistake, the amount is not added up at last, thus it is ignored.

Proof of Concept

In CollateralTracker contract on L711-720, it calculates the amount of value in loss in the chunk:

exerciseFees = exerciseFees.sub(
    LeftRightSigned
        .wrap(0)
        .toRightSlot(
            int128(uint128(oracleValue0)) - int128(uint128(currentValue0))
        )
        .toLeftSlot(
            int128(uint128(oracleValue1)) - int128(uint128(currentValue1))
        )
);

However at the end of the function when it calculates final amount of exercise fee, the calculated value is ignored:

exerciseFees = exerciseFees
    .toRightSlot(int128((longAmounts.rightSlot() * fee) / DECIMALS_128))
    .toLeftSlot(int128((longAmounts.leftSlot() * fee) / DECIMALS_128));

As shown above, by using toRightSlot and toLeftSlot, original exerciseFees value is overwritten by longAmounts * fee.

Tools Used

Manual Review

Recommended Mitigation Steps

The pre-calculated amount has to be added to the final amount so that:

exerciseFees = exerciseFees.add(
    LeftRightSigned
        .wrap(0)
        .toRightSlot(int128((longAmounts.rightSlot() * fee) / DECIMALS_128))
        .toLeftSlot(int128((longAmounts.leftSlot() * fee) / DECIMALS_128));
)

Assessed type

Context

dyedm1 commented 6 months ago

toRightSlot and toLeftSlot actually add (not overwrite) the input values to their respective slots

c4-judge commented 6 months ago

Picodes marked the issue as unsatisfactory: Invalid