search

code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

Truncate of values can be avoided #629

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L779 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L780 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L38-L40 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L64 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L38-L48

Vulnerability details

Truncate of values can be avoided

Summary

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Details

In general, this is a problem due to precision. In this case, it also affects assets, that makes me suggest High, as this operations are performed frecuently

Impact

Less fees, payouts, smaller prices and also size of positions may happen as result of this

Proof of Concept

Affecting fees and prices: https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L779 _daoFeesPaid = (_positionSize * _fees.daoFees / DIVISION_CONSTANT) * asset.feeMultiplier / DIVISION_CONSTANT

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L780

_burnFeesPaid = (_positionSize * _fees.burnFees / DIVISION_CONSTANT) * asset.feeMultiplier / DIVISION_CONSTANT

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L38-L40

First is called _initPositionSize = _margin * _leverage / 1e18

And then used to calculate the _payout _payout = int256(_margin) + int256(_initPositionSize * (1e18 * _currentPrice / _price - 1e18) / 1e18) + accInterest

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L64

_liqPrice = _tradePrice - ((_tradePrice * 1e18 / _leverage) * uint256(int256(_margin) + _accInterest) / _margin) * _liqPercent / 1e10

Not affecting assets directly: https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L38-L48

_initPositionSize = _margin * _leverage / 1e18

That then is used in: _positionSize = _initPositionSize * _currentPrice / _price

Tools

Slither + manual analysis

Recommended Mitigation Steps

Reorder the operations for avoiding lack of precision For example _daoFeesPaid = (_positionSize * _fees.daoFees / DIVISION_CONSTANT) * asset.feeMultiplier / DIVISION_CONSTANT

would be

_daoFeesPaid = (_positionSize * _fees.daoFees * asset.feeMultiplier) / (DIVISION_CONSTANT * DIVISION_CONSTANT)

GalloDaSballo commented 1 year ago

This is overly inflated (Rounding is typically Low) and it lacks a specific example of how rounding will cause issues

For these reasons am closing, either submit as QA or offer specific instances of the issue to demonstrate a High Impact

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity