Malicious protocol owner can steal all users funds (Centralization risk)

[code423n4]

Lines of code

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

Vulnerability details

Impact

The Tigris protocol does not have enough boundary conditions that prevent a malicious owner to steal users' funds.
By using some onlyOwner actions, it is possible, for example, to call Trading.setMaxWinPercent to any arbitrary value, including one greater than 100%, which would make it possible to open a position and close it with an arbitrarily high payout, allowing it to steal all the vault's funds.

Even if we trust the protocol owner, we believe this is an unnecessary and serious centralization risk. A malicious or compromised owner address can take advantage of this, and steal all the funds from the protocol.

Proof of Concept

1. Alice, a malicious protocol owner, opens a position, and immediately calls for Trading.setMaxWinPercent, setting the max win percent as an extremely large number, enough to empty the whole vault.
2. As a result, all the users' funds from the protocol are drained.

Tools Used

Manual analysis

Recommended Mitigation Steps

Impose boundary checks for all owner-controlled actions. For example, a maximum value for maxWinPercent. Review other functions with onlyOwner modifier and validate if the scope of these actions can be limited in order to reduce the damage of a potential incident.

[TriHaz]

We are aware of the centralization risks, owner of all contracts will be the multi-sig treasury for now, until we have a fully decentralized DAO.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[GalloDaSballo]

Admin privilege risk

[c4-judge]

GalloDaSballo marked the issue as satisfactory