search

code-423n4 / 2022-07-yield-findings

0 stars 0 forks source link

inkAtEnd (collateral) rounding error (divide before multiply) #166

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L594

Vulnerability details

Impact

Collateral rounding error

Proof of Concept

In the following line there is a division before multimplication. As artIn < auction_.art then this division will be zero.

    uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);@audit div before mul

Recommended Mitigation Steps

Multiply before divide 

[-]           uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);
[+]          uint256 inkAtEnd = uint256(artIn).wmul(auction_.ink).wdiv(auction_.art);
HickupHH3 commented 2 years ago

See libraries wdiv and wmul, it'll be doing multiplication then division.

ecmendenhall commented 2 years ago

Duplicate of https://github.com/code-423n4/2022-07-yield-findings/issues/28

HickupHH3 commented 2 years ago

~IMO it shouldn't be a duplicate; #28 does a much better job at explaining why the rounding error can still occur despite the code doing multiplication first, then division: (artIn * 1e18) / auction_.art * auction_.ink / 1e18.~

Edit: Nvm it isn't up to me to decide, should only be giving objective factual comments.

alcueca commented 2 years ago

It doesn't matter which finding is better described, they are both invalid.

uint256(artIn).wdiv(auction_.art).wmul(auction_.ink); is equivalent to artIn * 1e18 / auction_.art * auction_.ink / 1e18

PierrickGT commented 2 years ago

Agree with sponsor and Hickup, we do multiply first by 1e18 and then divide by 1e18, so it will cancel each other out. For this reason, I have labelled both issues as being invalid.

dmitriia commented 2 years ago

Agree with sponsor and Hickup, we do multiply first by 1e18 and then divide by 1e18, so it will cancel each other out. For this reason, I have labelled both issues as being invalid.

It's understood, but this doesn't look to be enough, please kindly read the description of #28

dmitriia commented 2 years ago

It doesn't matter which finding is better described, they are both invalid.

uint256(artIn).wdiv(auction_.art).wmul(auction_.ink); is equivalent to artIn * 1e18 / auction_.art * auction_.ink / 1e18

@alcueca The expressions aren't equivalent and there are a range of cases where precision loss leads to value rounding as it is shown in #28. I.e. both findings are valid. The severity is debatable, I believe as 1e18 does cover a substantial chunk of cases (but not all of them as you imply here), so it should be medium. It would be high if the precision was lost in majority of the cases

HickupHH3 commented 2 years ago

@dmitriia I thought about this, the issue would still persist regardless of order in a different manner.

The fix: artIn * auction_.ink / 1e18 * 1e18 / auction_.art will render zero in cases where the current implementation wouldn't: (artIn * 1e18) / auction_.art * auction_.ink / 1e18. Consider the case where

The fix would yield 1e10 * 1e6 / 1e18 * 1e18 / 1e15 = 0 while the current implementation would yield 1e10 * 1e18 / 1e15 * 1e6 / 1e18 = 10.

28 makes a point of the fyToken amount auction_.art being generally greater than 1e18, which I agree with. However, I think the likelihood of it exceeding 1e36 is very low.

dmitriia commented 2 years ago

@HickupHH3 Yes, you are right, the proper fix here is an introduction of additional precision enhancing multiplier, switching wdiv with wmul just reduces the probability of encountering the issue, altering, but not removing the surface.

The issue itself can be formulated as 'using wad math isn't enough to fully counter division precision loss'. The real life probability might be low, but these two issues aren't invalid, just describe a corner case (as many others actually are, which is natural, as main case is usually already thought of)

PierrickGT commented 2 years ago

@dmitriia @HickupHH3 thanks for looking into this issue.

@GalloDaSballo wrote a script to compare the two order of operations and noticed that the rounding difference does exist but it's in an order of magnitude of 1 billionth of a ETH (1e9), which is negligible. For this reason, I will remove the invalid label and downgrade this issue to a QA Report one.

The script is available here: https://gist.github.com/GalloDaSballo/1f86b550b45975445848bec61353f45e And here is the result: unknown