Compromised or malicious owner of Trading
contract can set fees to be bigger than 100% for blocking users from taking important trading actions, such as initiating closing position
#641
Labels
bug
Something isn't working
downgraded by judge
Judge downgraded the risk level of this issue
duplicate-658
primary issue
Highest quality submission among a set of duplicates
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Lines of code
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L952-L969
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L163-L210
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810
Vulnerability details
Impact
When calling the following
Trading.setFees
function to set fees for opening and closing positions, there are no upper limits for these fees. If the owner of theTrading
contract becomes compromised or malicious, this owner can set these fees to be more than 100%. When this happens, as shown below, calling functions likeTrading.initiateMarketOrder
andTrading._handleCloseFees
can revert due to underflowed arithmetic operations caused by the high fees that are more than 100%. It is possible that the fees for opening position are set to normal values but the fees for closing position are set to values that are larger than 100%. In this case, for example, a user can initiate a market order but will fail to initiate closing position for this market order. As a result, this user is forced to lose the deposited margin.https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L952-L969
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L163-L210
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810
Proof of Concept
Please add the following test in the
Trading using <18 decimal token
describe
block intest\07.Trading.js
. This test will pass to demonstrate the described scenario. Please see the comments in this test for more details.Tools Used
VSCode
Recommended Mitigation Steps
In the
Trading.setFees
function, each fee, which would be set, needs to be capped below a sensible upper limit that is less than 100%.The text was updated successfully, but these errors were encountered: