Volt Protocol contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/oracle/ScalingPriceOracle.sol#L136 https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/oracle/ScalingPriceOracle.sol#L113

Vulnerability details

Impact

The oracle does not correctly compound the monthly APRs - it resets on fulfill. Note that the oraclePrice storage variable is only set in _updateCPIData as part of the oracle fulfill callback. It's set to the old price (price from 1 month ago) plus the interpolation from startTime to now. However, startTime is reset in requestCPIData due to the afterTimeInit modifier, and therefore when Chainlink calls fulfill in response to the CPI request, the timeDelta = block.timestamp - startTime is close to zero again and oraclePrice is updated to itself again.

This breaks the core functionality of the protocol as the oracle does not track the CPI, it always resets to 1.0 after every fulfill instead of compounding it. In addition, there should also be a way for an attacker to profit from the sudden drop of the oracle price to 1.0 again.

POC

As an example, assume oraclePrice = 1.0 (1e18), monthlyAPR = 10%. The time elapsed is 14 days. Calling getCurrentOraclePrice() now would return 1.0 + 14/28 * 10% = 1.05.

• it's now the 15th of the month and one can trigger requestCPIData. This resets startTime = now.
• Calling getCurrentOraclePrice() now would return 1.0 again as timeDelta (and priceDelta) is zero: oraclePriceInt + priceDelta = oraclePriceInt = 1.0.
• When fulfill is called it sets oraclePrice = getCurrentOraclePrice() which will be close to 1.0 as the timeDelta is tiny.

Recommended Mitigation Steps

The oraclePrice should be updated in requestCPIData() not in fulfill. Cover this scenario of multi-month accumulation in tests.

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/RateLimited.sol#L142

Vulnerability details

Impact

The RateLimited.setBufferCap function first updates the buffer and then sets the new cap, but does not apply the new cap to the updated buffer. Meaning, the updated buffer value can be larger than the new buffer cap which should never be the case. Actions consuming more than the new buffer cap can be performed.

function _setBufferCap(uint256 newBufferCap) internal {
    // @audit still uses old buffer cap, should set buffer first
    _updateBufferStored();

    uint256 oldBufferCap = bufferCap;
    bufferCap = newBufferCap;


    emit BufferCapUpdate(oldBufferCap, newBufferCap);
}

Recommended Mitigation Steps

Update the buffer after setting the new cap:

function _setBufferCap(uint256 newBufferCap) internal {
-   _updateBufferStored();
    uint256 oldBufferCap = bufferCap;
    bufferCap = newBufferCap;

+   _updateBufferStored();

    emit BufferCapUpdate(oldBufferCap, newBufferCap);
}

Judge @jack-the-pug is upgrading the following issue from a QA report (issue #30 ) to Medium risk:

Division by zero in isWithinDeviationThreshold if a is zero. This only seems to be the case if the oracle would return 0 for CPI and in this case, something is wrong anyway. Should still handle this error more gracefully.

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/refs/OracleRef.sol#L104

Vulnerability details

Impact

The OracleRef assumes that the backup oracle uses the same normalizer as the main oracle. This generally isn't the case as it could be a completely different oracle, not even operated by Chainlink.

If the main oracle fails, the backup oracle could be scaled by a wrong amount and return a wrong price which could lead to users being able to mint volt cheap or redeem volt for inflated underlying amounts.

Recommended Mitigation Steps

Should there be two scaling factors, one for each oracle?

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/MultiRateLimited.sol#L280

Vulnerability details

Impact

When the bufferCap is updated for an address in _updateAddress, the address's allowed buffer (bufferStored) is replenished to the entire bufferCap.

The address could frontrun the updateAddress call and spend their entire buffer, then the buffer is replenished and they can spend their entire buffer a second time.

Recommended Mitigation Steps

Keep the old buffer value, capped by the new bufferCap:

+ uint256 newBuffer = individualBuffer(rateLimitedAddress);

  rateLimitData.lastBufferUsedTime = block.timestamp.toUint32();
  rateLimitData.bufferCap = _bufferCap;
  rateLimitData.rateLimitPerSecond = _rateLimitPerSecond;
- rateLimitData.bufferStored = _bufferCap;
+ rateLimitData.bufferStored = min(_bufferCap, newBuffer);

QA

• DeviationThresholdUpdate never emitted
• Division by zero in isWithinDeviationThreshold if a is zero. This only seems to be the case if the oracle would return 0 for CPI and in this case, something is wrong anyway. Should still handle this error more gracefully.