handling of `MarketStatus.FORBIDDEN` is unsound in `FuturesLogic::getDeleverageObject()` function

zlsecure3 commented 1 year ago

subject

handling of MarketStatus.FORBIDDEN is unsound in FuturesLogic::getDeleverageObject() function

description

        if (marketConfig.getStatus() != DataTypes.MarketStatus.FORBIDDEN) {
            require(absSkewness > skewnessSoftCap);
            require(isLongSkewed == positionSide);

            orderQty = Math.min(absSkewness - skewnessSoftCap, orderQty);
        }

If marketConfig.getStatus() == DataTypes.MarketStatus.FORBIDDEN, the above require statement won't be check, which means it's possible that the sknewness becomes even larger or absSkewness has not overceeded skewnessSoftCap.

And the comment of deleverage says deleverage is supposed to be called when the market is FORBIDDEN, which doesn't match the implementation.

recommendation

Make the comment and implementation consistent.

locations

code/contracts/core/managers/FuturesManager.sol#L401
code/contracts/libraries/logics/FuturesLogic.sol#L215-L220

severity

Medium

damage

exploitability

grading (edit)

submission_id:1772394099

review_type:GRADING

result: TBD-yes,no

rating: TBD-123

comment: TBD-Rejected,Accepted by Secure3.

severity: TBD-Critical,Medium,Low,Informational

category:

description:

zlsecure3 commented 1 year ago

client feedback (manual copy)

submission_id:1772394099

review_type:CLIENT_FEEDBACK

result: TBD-yes,no

severity: TBD-Critical,Medium,Low,Informational

comment:

zlsecure3 commented 1 year ago

client feedback decision(edit)

submission_id:1772394099

review_type:CLIENT_FEEDBACK_DECISION

result: TBD-yes,no,yes-honored,no-honored

severity: TBD-Critical,Medium,Low,Informational

comment:

zlsecure3 / review_Aark