Yield Witch v2 contest - peritoflores's results

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

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

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.

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

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.

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

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

@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

• artIn = 1e10
• auction_.ink = 1e6
• auction_.art = 1e15

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.

@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)

@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: