code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/PairsContract.sol#L52 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L38 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L36

Vulnerability details

Impact

Unexpected behavior during trading can be caused due to _maxLeverage being set to a very high (near overflow) value. Note that most of the other variables (percent fee, base funding rate, etc) have bounds on being set.

Proof of Concept

-In addAsset() the only check on _maxLeverage is that it is greater than _minLeverage https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/PairsContract.sol#L52 -If the _maxLeverage were somehow set to a near overflow value, this would cause unexpected behavior during trading. -For example, it could cause an overflow in the downstream pnl() function via https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L38 which is used in many core functions in Trading. -Another example is in reducePosition() the initId[_id] could be overflowed via https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L233 and this causes the accInterest field on every trade to be unpredictable https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L62 which also leads to issues via downstream effects to pnl() https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L36. -

Tools Used

None

Recommended Mitigation Steps

Add in a check on the range of values that maxLeverage can be set to.

TriHaz commented 1 year ago

I don't think this is a valid med risk issue, only addAsset() should have a check so I think this should be QA.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

TriHaz marked the issue as disagree with severity

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/607

GalloDaSballo commented 1 year ago

Downgrading to QA because the finding is valid but I agree that it ultimately is a lack of check in the setter