search

code-423n4 / 2022-11-looksrare-findings

0 stars 0 forks source link

Calculation of fee should be rounded up to avoid rounding error #144

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/SeaportProxy.sol#L146-L147 https://github.com/code-423n4/2022-11-looksrare/blob/main/contracts/proxies/SeaportProxy.sol#L207

Vulnerability details

Impact

It will reduce the normal income of the project.

Proof of Concept

Fee is calcuated like this in the code:

 uint256 orderFee = (price * feeBp) / 10000;

It uses rounding down division, which will reduce the fee when rounding occurs .

Tools Used

Recommended Mitigation Steps

Any tokens taken from users should use rounding up division.

Picodes commented 1 year ago

Why would it be better to round up at the benefit of the protocol rather than users ?

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

5z1punch commented 1 year ago

Why would it be better to round up at the benefit of the protocol rather than users ?

oops, I think it's a kind of good development practice. Because take 1 unit from a user, user just loss a dust. But if one thousand users call the function one thousand times, losses to the project will be accumulated. It's a real bug fixed in the Curve.Fi https://github.com/curvefi/curve-contract/blob/b0bbf77f8f93c9c5f4e415bce9cd71f0cdee960e/contracts/pool-templates/base/SwapTemplateBase.vy#L466 . And it even caused huge losses on the solana project https://osec.io/blog/reports/2022-04-26-spl-swap-rounding/ . It's okey for rejecting the issue. But I think the unsatisfactory tag could be a misunderstanding which is worth my while to explain in detail. Hope that will be helpful. Thanks!