sherlock-admin
commented
11 months ago 4 comment(s) were left on this issue during the judging contest.
panprog commented:
invalid because such pending positions can not happen (adding the last pending position of 8 will revert) due to the new condition of only being able to close what is available from the latest position minus all pending negative deltas (so, when trying to add pending position of 8: closableAmount = 20(latest) - (30-15) - (15-8) = 20-15-7 = -2 -> it will revert, so such pending position can not happen.
n33k commented:
invalid, not clearly showing how it could enter such revert state, suggest providing a PoC test case if escalate
darkart commented:
Since solidity 8.0 it will stop at 0
polarzero commented:
Invalid. This does not seem to immediately put any funds at risk, and the malfunction would be limited.
tvdung94
high
closableAmount could go below zero, causing revert when loading pending positions
Summary
closableAmount could go below zero, causing revert when loading pending positions.
Vulnerability Detail
closableAmount is not calculated correctly and could go below zero in some cases. Example: latestPosition magnitude = 20 pendingLocalPositions magnitude = [30, 15 , 8]
i = 0 (First iterate): closeableAmountBefore = 20, previousMagnitude = 20 closeableAmountAfter = 20 - (20 - min( 30, 20)) = 20 - (20 - 20) = 20 - 0 = 20
i = 1 (Second iterate): closeableAmountBefore = 20, previousMagnitude = 30 closeableAmountAfter = 20 - (30 - min( 15, 30)) = 20 - (30 - 15) = 20 -15 = 5
i = 2 (Third iterate): closeableAmountBefore = 5, previousMagnitude = 15 closeableAmountAfter = 5 - (15 - min( 8, 15)) = 5 - (15 - 8) = 5 - 7 = -2 (which is below 0)
Impact
System DOS in some cases
Code Snippet
https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L563-L595
Tool used
Manual Review
Recommendation
Revise closableAmount formula