code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L122-L147 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L97 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L136-L138 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L177 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L230 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L246 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L272

Vulnerability details

Prevent div by 0

Impact

On several locations in the code precautions are taken not to divide by 0, because this will revert the code. However on some locations this isn’t done.

All this reverts can be caused by state variables assigned to 0 or with length 0, this will cause system to stop working on the functions with this divisions until variables are modified.

For example in PRICE.updateMovingAverage():

    // Cache numObservations to save gas.
    uint256 numObs = observations.length;

  _movingAverage = total / numObs; //@audit division by 0 if in constructor observations is initialized to length 0

Reference of the code: https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L122-L147

Proof of Concept

Navigate to the following contracts, https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L97 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L136-L138 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L177 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L230 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L246 https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L272

In multiple occasions variable can be set to 0, therefore div by zero will occur.

Recommended Mitigation Steps

Recommend making sure division by 0 won’t occur by checking the variables beforehand.

Oighty commented 2 years ago

Most of the ones listed are either checked when the variable is set or redundant since the system will not work if the value is set to zero to begin with (e.g. observationFrequency).

The only legitimate one I can see is https://github.com/code-423n4/2022-08-olympus/blob/549b96bcf8b97807738572605f6b1e26b33ef411/src/modules/PRICE.sol#L177, which is a duplicate of part of the Chainlink validation reports, e.g. #441

0xean commented 1 year ago

Closing as invalid, warden would have needed to demonstrate how the state needed for division by 0 to occur would have occurred to be a valid finding.

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