giraffe - Unsafe casting of Int256 perpNetValue leads to DOS

[sherlock-admin2]

giraffe

medium

Unsafe casting of Int256 perpNetValue leads to DOS

Summary

In FundingRateArbitrage.sol, the function getNetValue() does SafeCast.toUint256(perpNetValue). When perpNetValue is negative, this cast will revert.

Vulnerability Detail

function getNetValue() public view returns (uint256) {
        ...
        (int256 perpNetValue,,,) = JOJODealer(jojoDealer).getTraderRisk(address(this));
        
        //@audit when perpNetValue is negative Safecast will revert
        return
            SafeCast.toUint256(perpNetValue) + collateralAmount.decimalMul(collateralPrice) + usdcBuffer - jusdBorrowed;
    }

perpNetValue can be a negative value when the trades (by contract Owner) are losing money (confirmed with Sponsor). This will result in SafeCast reverting:

function toUint256(int256 value) internal pure returns (uint256) {
        require(value >= 0, "SafeCast: value must be positive");
        return uint256(value);
    }

Impact

All deposit and withdraw functions in FundingRateArbitrage.sol are DOS-ed as they rely on getNetValue() and will revert when called. The impact is significant because users are likely to want to withdraw if the trades are losing money, but are unable to do so due to the bug described.

Code Snippet

https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L94

Tool used

Manual Review

Recommendation

Add additional if/else conditions in deposit and withdraw to handle a negative perpNetValue scenario and possible overall negative netValue scenario.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { It will not revert as its in int256 which is to hold negative values}

[detectiveking123]

Escalate

This issue is invalid. If you look at getTraderRisk function, then it computes the TOTAL value of the position (including margin), not whether they're just up or down.

In order to get to this state, the FundingRateArbitrage contract, whose positions are managed by admins:

1. Must have incurred bad debt
2. Must not have been liquidated for whatever reason

Furthermore, even after we get to this position, the contract can still be liquidated and the bad debt can be taken care of.

[sherlock-admin]

Escalate

This issue is invalid. If you look at getTraderRisk function, then it computes the TOTAL value of the position (including margin), not whether they're just up or down.

In order to get to this state, the FundingRateArbitrage contract, whose positions are managed by admins:

1. Must have incurred bad debt
2. Must not have been liquidated for whatever reason

Furthermore, even after we get to this position, the contract can still be liquidated and the bad debt can be taken care of.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Hash01011122]

Correct me if I am wrong
Building upon the points raised during the escalation, it’s important to note that the impact of this issue is minimal. In the event that a user’s perpNetValue goes negative, the transaction would simply revert without any loss of funds or the contract getting bricked.
Moreover, the probability of this scenario occurring is quite low. For this to happen, a user would need to have bad debt or not have incurred any liquidation, which is not the situation we’re dealing with, as highlighted during the escalation.
This is low-informational severity.

[nevillehuang]

I believe even though this would require a trusted admin action on trades, it should not explicitly DoS core functionalities like depositing and requesting withdrawal. While this is an edge case, if theres a bad trade causing the value to fall to negative, why wouldn't the protocol be open to more collateral deposits?

If any of the watsons here can provide me a reasonable numerical example that this will never realistically occur, I will considering lowering severity, if not I believe the severity is correct.

[giraffe0x]

To add on: Funding Rate Arbitrage is advertised by JOJO as a low-risk, stable return product. In the unfortunate event that positions are negative, admins may decide to inject funds via deposit to avoid liquidation. However, it is at this critical moment deposit() reverts due to the described bug, allowing for liquidation and losses to JOJO's users.

[detectiveking123]

@nevillehuang @giraffe0x I believe you are misunderstanding the point. getTraderRisk does not return a trader's PNL. It returns the value of their overall position, including margin. They should have been liquidated already, before bad debt is incurred. Even if bad debt is incurred, the situation can be resolved through a liquidation and then bad debt resolution.

[nevillehuang]

@JoscelynFarr Can you confirm @detectiveking123 comments? I might have missed protocol mechanisms revolving around liquidation mechanisms given the limited time I have to judge a huge update contest. If it is true that admin/any liquidator can resolve issue via a liquidation, I can agree that it is low severity.

[JoscelynFarr]

Agree with @detectiveking123. No need to fix the issue. Will remove the fix label

[JoscelynFarr]

@JoscelynFarr Can you confirm @detectiveking123 comments? I might have missed protocol mechanisms revolving around liquidation mechanisms given the limited time I have to judge a huge update contest. If it is true that admin/any liquidator can resolve issue via a liquidation, I can agree that it is low severity.

getTraderRisk does not return a trader's PNL. It returns the value of their overall position, including margin. which is true, so I think before it becomes negative. It would be liquidated
.

[nevillehuang]

@JoscelynFarr Can you point to the code logic that performs this liquidation? Once I verify I agree this can be low/invalid severity.

[JoscelynFarr]

@JoscelynFarr Can you point to the code logic that performs this liquidation? Once I verify I agree this can be low/invalid severity.

Sure getTraderRisk function returns users' netValue which includes USDC + JUSD + netPositionValue
https://github.com/JOJOexchange/smart-contract-EVM/blob/main/src/libraries/Liquidation.sol#L69

The liquidation formulation can be find in here
https://github.com/JOJOexchange/smart-contract-EVM/blob/main/src/libraries/Liquidation.sol#L189
So if netValue < SafeCast.toInt256(maintenanceMargin); then the user will be liquidated. So netValue will bigger than zero.

[Evert0x]

@nevillehuang did you verify it?

[nevillehuang]

Yes @Evert0x can be invalid, but what I can tell you is if it so happens nobody is willing to liquidate, core functionalities like depositing will indeed be bricked. However, I trust that admin will liquidate appropriately to unblock DoS

[Evert0x]

Ok planning to accept escalation and invalidate issue

[Czar102]

Result:
Invalid
Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• detectiveking123: accepted