code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L237 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L254

Vulnerability details

Impact

Deactived checked can be deliver wrong return

Proof of Concept

When auctioneer was live, so range.low.market and range.high.market was checked is back above the cushion and if the price is below the wall

the only way was used && since if the case was the first condition was true and second was false the return was true both if the condition was false, then both of them was false :

example :

            if (auctioneer.isLive(range.low.market)) {
                if (currentPrice > range.cushion.low.price || currentPrice < range.wall.low.price) {
                    _deactivate(false);
                }

so if currentPrice > range.cushion.low.price -> true then second condition was true but if currentPrice > range.cushion.low.price -> false then second condition can be true

if there was a case that first condition was false, u didn't need to check if currentPrice < range.wall.low.price since it was needed that one condition to be false to open a new bond market. So if either currentPrice > range.cushion.low.price or currentPrice < range.wall.low.price just only needed to check it once. Since if neither first or second condition can be done by just one deliver only.

Tools Used

Manual Review

Recommended Mitigation Steps

By change || into && operator

Oighty commented 2 years ago

range.cushion.low.price > range.cushion.low.wall should always be true. Therefore, if you change the referenced line to currentPrice > range.cushion.low.price && currentPrice < range.wall.low.price, it could never be true and this code would never run. The current implementation is correct. This is not a bug.

0xean commented 2 years ago

closing as invalid.

code-423n4 / 2022-08-olympus-findings