sherlock-admin
commented
1 year ago 2 comment(s) were left on this issue during the judging contest.
141345 commented:
x
panprog commented:
low (invalid) as it seems it's by design to specifically let the user open positions which will be settled later and impact is also at most low
feelereth
high
collateralAfterFees does not subtract the fees for the current position
Summary
In the _loadPendingPositions() function. It calculates collateralAfterFees by subtracting the fees for pending positions up to but not including the current position.
Vulnerability Detail
The _loadPendingPositions() function loads all the pending positions into memory for checking collateralization in _invariant(). It calculates collateralAfterFees by subtracting the fees from previous pending positions: Link
However, it does not subtract the fees for the current position, which is added separately Link2
This allows a malicious actor to take a position that appears collateralized based on collateralAfterFees, but actually becomes undercollateralized once the current position's fees are applied. For example: • Account collateral: 100 • Pending fee: 5 • Current position fee: 10 collateralAfterFees would be 100 - 5 = 95 The position would appear collateralized with 95 . But once the 10 current fee is applied, the true collateral is only 85. This could allow an undercollateralized position to pass the collateralization check in _invariant().
Impact
It allows a malicious actor to take a position that appears collateralized based on collateralAfterFees, but actually becomes undercollateralized once the current position's fees are applied.
Code Snippet
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L549-L558
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L559
Tool used
Manual Review
Recommendation
_loadPendingPositions() should also subtract the current position's fees