Centralization allows owner to withhold user payouts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L926-L933

Vulnerability details

Impact

The owner of the contract could set the maxWinPercent state variable in the Trading contract to something like 0.000001%, which will withhold basically all user deposited funds (for open market orders) inside the StableVault contract. See here for the usage of maxWinPercent.

The impact of this is reduced due to two circumstances:

1. The owner is a multisig Gnosis wallet
2. There isn't a way for the owner to rug the StableVault contract (as far as I can see)

Regardless, since the payout includes the initial amount the user invested into the order, the owner is able to basically trap user funds with an extremely low maxWinPercent. The impact is the same if the multisig wallet is somehow compromised.

Proof of Concept

The owner would simply need to call setMaxWinPercent() (See here) with a value of 1. Closing any market orders after that would return a negligible amount of the initially deposited user funds, no matter whether the user is supposed to have made a profit or a loss.

Let me know if a PoC for this is actually necessary.

I mentioned above that the payout includes the initial amount that the user invested into the order. We know this because the order of function calls required to get to the usage of maxWinPercent (linked in the "Impact" section above) is (and this is just one example):

1. initiateCloseOrder()
2. _closePosition().

Inside _closePosition(), the code uses tradingExtension._closePosition() to get the payout. See here.

Looking at TradingExtension, this function will use the TradingLibrary contract's pnl() function to calculate the payout:

• tradingExtension._closePosition()
• TradingLibrary.pnl()

Looking at the pnl() function, it uses the order's position size, opening price, margin, and the current price to calculate the payout. This means it includes the entire amount that the user deposited for the order.

Tools Used

N/A

Recommended Mitigation Steps

Set a minimum limit for the maxWinPercent in setMaxWinPercent().

Better yet, remove this function. I don't see a purpose to this function. Why would you want to prevent users from earning their winnings? Trust in the platform itself is lessened with a function like this in the contract, even with a minimum limit set.

[TriHaz]

As mentioned in other centralization issues, we are aware of the risk and contracts owner will be a multi-sig treasury for now.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[GalloDaSballo]

Admin privilege denial of yield

[c4-judge]

GalloDaSballo marked the issue as satisfactory