sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

Nyx - Borrowers can liquidated when they are not in bad debt. #146

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Nyx

medium

Borrowers can liquidated when they are not in bad debt.

Summary

Borrowers can be liquidatable when they are not in bad debt.

Vulnerability Detail

To liquidate any account, liquidate() function checks that the borrower maintenance is greater than user collateral or not. Borrowers can be liquidated if the borrower's maintenance is greater than the collateral.

function liquidatable(address account, IProduct product) external view returns (bool) {
        if (product.isLiquidating(account)) return false;

        return product.maintenance(account).gt(collateral(account, product));
    } 

the liquidatable() function checks that correctly.

if (!totalMaintenance.gt(totalCollateral))
            revert CollateralCantLiquidate(totalMaintenance, totalCollateral); 

But liquidate() function is not checking that correctly. When the borrower maintenance is equal to the borrower collateral, the position should be safe and liquidate() function should revert.

Impact

Early liquidation. Borrowers can lose their collaterals.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/collateral/Collateral.sol#L120-L121

Tool used

Manual Review

Recommendation

if (totalCollateral >= totalMaintenance)
            revert CollateralCantLiquidate(totalMaintenance, totalCollateral); 

Or check with the liquidatable() function.

arjun-io commented 1 year ago

The two checks, product.maintenance(account).gt(collateral(account, product)); and !totalMaintenance.gt(totalCollateral) are equivalent

a) if (maintenance > collateral) liquidatable = True
b) if (!(maintenance > collateral) liquidatable = False

In the equal case

totalMaintenance = 100
totalCollateral = 100
if (!(maintenance > collateral)) revert NotLiquidatable
   i.e. (!(100 > 100))  revert NotLiquidatable
   i.e. (!(False))   revert NotLiquidatable
KenzoAgada commented 1 year ago

I believe this is what is termed a brainfart from my end