sherlock-audit / 2023-11-olympus-judging

9 stars 8 forks source link

detectiveking - `operate()` is missing a case when activating / deactivating #161

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 9 months ago

detectiveking

high

operate() is missing a case when activating / deactivating

Summary

operate() is missing a case when activating / deactivating the range

Vulnerability Detail

In operate(), there are a bunch of if statements that activate and deactivate the range and auctioneer market based on the price. Here are the if statements for the low market:

        if (range.low.active) {
            if (auctioneer.isLive(range.low.market)) {
                // if active, check if the price is back above the cushion
                // or if the price is below the wall
                // if so, close the market
                if (currentPrice > range.low.cushion.price || currentPrice < range.low.wall.price) {
                    _deactivate(false);
                }
            } else {
                // if not active, check if the price is below the cushion
                // if so, open a new bond market
                if (currentPrice < range.low.cushion.price && currentPrice > range.low.wall.price) {
                    _activate(false, currentPrice);
                }
            }
        }

However, there is a case missing here. In the event that we have range.low.active but the auctioneer market is down (so we go into the else statement), we check:

                if (currentPrice < range.low.cushion.price && currentPrice > range.low.wall.price) {
                    _activate(false, currentPrice);
                }

However, this needs another case. We need to deactivate the range in the event that the price is not in the range where we should activate. The correct code would look something like this:

        if (range.low.active) {
            if (auctioneer.isLive(range.low.market)) {
                // if active, check if the price is back above the cushion
                // or if the price is below the wall
                // if so, close the market
                if (currentPrice > range.low.cushion.price || currentPrice < range.low.wall.price) {
                    _deactivate(false);
                }
            } else {
                // if not active, check if the price is below the cushion
                // if so, open a new bond market
                if (currentPrice < range.low.cushion.price && currentPrice > range.low.wall.price) {
                    _activate(false, currentPrice);
                } else {
                   range.low.active = false;
                }
            }
        }

Impact

The market will stay up when it should be down, meaning people will still be able to swap against it.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/policies/RBS/Operator.sol#L267-L282

Tool used

Manual Review

Recommendation

See above

nevillehuang commented 9 months ago

Invalid, else block is only executed when range.low.active = false, so no issue here since if new bond market is not opened in the if statement here, range.low.active = false will simply remain as false