search

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

5 stars 4 forks source link

Operator: if WallSpread is 10000, `operate` and `beat` will revert and price information cannot be updated anymore #404

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

Vulnerability details

Impact

The beat cannot be called anymore and price information will not be updated

Condition:

Proof of Concept

The below proof of concept demonstrates that the operate will revert with 100% wallspread. The full test code can be found here as well as the diff from Operator.t.sol.

In the test, the wallspread was set to 10000, which is 100% (line 51). The price was set so that the lower market should be deployed (line 59). In the market deployment logic (Operator::_activate) will revert due to division by zero, and operate will fail.

Once this condition is met, the operate cannot be called and Heart::beat cannot be called as well, since the Heart::beat is calling Operator::opearate under the hood. As the result the price can never be updated. But other codes who uses the price information will not know that the price information is stale. If the upper wall is active and still have the capacity, one can swap from the upper wall using the stale information, which might cause some loss of funds.

    function test_poc__lowCushionDeployWithWallspread10000Reverts() public {
        /// Initialize operator
        vm.prank(guardian);
        operator.initialize();

        /// if the wallspread is 10000 the operate might revert
        vm.prank(policy);
        operator.setSpreads(7000, 10000);

        /// Confirm that the cushion is not deployed
        assertTrue(!auctioneer.isLive(range.market(true)));

        /// Set price on mock oracle into the lower cushion
        /// given the lower wallspread is 10000
        /// when the lower market should be deployed the operate reverts
        price.setLastPrice(20 * 1e18);

        /// Trigger the operate function manually
        /// The operate will revert Error with "Division or modulo by 0"
        /// But I could not figure out to catch with `expectRevert`
        /// so just commenting out the assert
        // vm.prank(guardian);
        // /// vm.expectRevert(bytes("Division or module by 0"));   // this cannot catch the revert...
        // operator.operate();
    }

Cause

The main cause is that the RANGE::setSpreads function fails to check for wallSpread_ == 10000. If the setter does not allow the wallSpread to be 100%, the price of the lower wall will not go to zero.

// modules/RANGE.sol
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
245             wallSpread_ > 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();

In the RANGE::updatePrices, the price of lower wall will be zero if the wallSpread is 100%. If the price of lower wall is zero, the Operator::_activate will fail for the lower cushion.

// policies/Operator.sol::_activate(bool high_)
// when high_ is false
421             uint256 invWallPrice = 10**(oracleDecimals * 2) / range.wall.low.price;

// modules/RANGE.sol::updatePrices
164         _range.wall.low.price = (movingAverage_ * (FACTOR_SCALE - wallSpread)) / FACTOR_SCALE;

Tools Used

foundry

Recommended Mitigation Steps

Mitigation suggestion: line 245. Forbid wallSpread to be 100%.

// modules/RANGE.sol
// https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/RANGE.sol#L242-L250

242     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243         // Confirm spreads are within allowed values
244         if (
-245            wallSpread_ > 10000 ||
+               wallSpread_ >= 10000 ||
246             wallSpread_ < 100 ||
247             cushionSpread_ > 10000 ||
248             cushionSpread_ < 100 ||
249             cushionSpread_ > wallSpread_
250         ) revert RANGE_InvalidParams();
Oighty commented 2 years ago

This is indeed an edge case and we will update the value checks for the spread values to exclude 10000. However, from a practical perspective, this is very unlikely to happen. If the goal is to set the lower wall to 0, then the system would just be disabled.

0xean commented 2 years ago

given the warden does fully demonstrate the issue I am going to award as M with the understanding that this is an extreme edge case.