Users can bypass the maxWinPercent limit using a partially closing

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L625-L627

Vulnerability details

Impact

Users can bypass the maxWinPercent limit using a partial closing.

As a result, users can receive more funds than their upper limit from the protocol.

Proof of Concept

As we can see from the documentation, there is limitation of a maximum PnL.

Maximum PnL is +500%. The trade won't be closed unless the user sets a Take Profit order or closes the position manually.

And this logic was implemented like below in _closePosition().

File: 2022-12-tigris\contracts\Trading.sol
624:                 _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);
625:                 if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) { //@audit bypass limit
626:                     _toMint = _trade.margin*maxWinPercent/DIVISION_CONSTANT;
627:                 }

But it checks the maxWinPercent between the partial payout and full margin so the below scenario is possible.

1. Alice opened an order of margin = 100 and PnL = 1000 after taking closing fees.
2. If maxWinPercent = 500%, Alice should receive 500 at most.
3. But Alice closed 50% of the position and she got 500 for a 50% margin because it checks maxWinPercent with _toMint = 500 and _trade.margin = 100
4. After she closed 50% of the position, the remaining margin = 50 and PnL = 500 so she can continue step 3 again and again.
5. As a result, she can withdraw almost 100% of the initial PnL(1000) even though she should receive at most 500.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check the maxWinPercent between the partial payout and partial margin like below.

    _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);

    uint256 partialMarginToClose = _trade.margin * _percent / DIVISION_CONSTANT; //+++++++++++++++++++++++
    if (maxWinPercent > 0 && _toMint > partialMarginToClose*maxWinPercent/DIVISION_CONSTANT) { 
        _toMint = partialMarginToClose*maxWinPercent/DIVISION_CONSTANT;
    }

[c4-judge]

GalloDaSballo marked the issue as duplicate of #111

[TriHaz]

I don't think any of this, #339, #487 is a duplicate of #111
But I would label this as primary for better mitigation.
Also I would label it as med risk as a +500% win is required so assets are not in a direct risk.
@GalloDaSballo

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[c4-sponsor]

TriHaz requested judge review

[GalloDaSballo]

Thank you for flagging @TriHaz , will re-dedoup

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[GalloDaSballo]

@TriHaz I see your point, and agree that the findings are different, thank you for the flag

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

The Warden has shown how, by partially closing an order, it is possible to bypass the maxWinPercent cap.

Per similar discussion to #111 the fact that not every trade can be above 500% in payout is not a guarantee that some trade will be, and those that will, will cause the invariant to be broken and LPs to be deeper in the red than they should.

Because this causes an immediate gain to the attacker, at a loss for LPs, I agree with High Severity.

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GainsGoblin]

Mitigation: code-423n4/2022-12-tigris#2 (comment)