The Asset.lotPrice doubles the oracle timeout in the worst case

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/Asset.sol#L139-L145

Vulnerability details

When the tryPrice() function revert, for example oracle timeout, the Asset.lotPrice will use a decayed historical value:

uint48 delta = uint48(block.timestamp) - lastSave; // {s}
if (delta <= oracleTimeout) {
    lotLow = savedLowPrice;
    lotHigh = savedHighPrice;
} else if (delta >= oracleTimeout + priceTimeout) {
    return (0, 0); // no price after full timeout
} else {

And the delta time is from the last price saved time. If the delta time is greater than oracle timeout, historical price starts decaying.

But the last price might be saved at the last second of the last oracle timeout period. So the Asset.lotPrice will double the oracle timeout in the worst case.

Impact

The Asset.lotPrice will double the oracle timeout in the worst case. When the rewards need to be sold or basket is rebalancing, if the price oracle is offline temporarily, the Asset.lotPrice will use the last saved price in max two oracle timeout before the historical value starts to decay. It increases the sale/buy price of the asset.

Proof of Concept

The lastSave is updated in the refresh() function, and it's set to the current block.timestamp instead of the updateTime from the chainlink feed:

function refresh() public virtual override {
    try this.tryPrice() returns (uint192 low, uint192 high, uint192) {
        if (high < FIX_MAX) {
            savedLowPrice = low;
            savedHighPrice = high;
            lastSave = uint48(block.timestamp);

But in the OracleLib, the oracle time is checked for the delta time of block.timestamp - updateTime:

uint48 secondsSince = uint48(block.timestamp - updateTime);
if (secondsSince > timeout) revert StalePrice();

So if the last oracle feed updateTime is block.timestamp - priceTimeout, the timeout check will be passed and lastSave will be updated to block.timestamp. And the lotPrice will start to decay from lastSave + priceTimeout. However when it starts, it's been 2 * priceTimeout since the last oracle price update.

Tools Used

Manual review

Recommended Mitigation Steps

Starts lotPrice decay immediately or updated the lastSave to updateTime instead of block.timestamp.

Assessed type

Context

[c4-judge]

thereksfour marked the issue as primary issue

[tbrent]

This issue was known and was discussed internally. Unfortunately this occurred in the private copy of the repo that the devs use to coordinate while C4 contests are ongoing. I've attached a screenshot of the discussion, though it is up to C4 how to treat this ultimately. We could probably provide repo access to a member of the C4 team if asked.

We decided not to pursue this direction as it introduced a large number of changes, and it seems acceptable to have the worst-case behavior of using 100% of the last saved price for up to one oracleTimeout too long.

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[thereksfour]

Agree that sponsors not address it. And this issue will be considered as medium risk under the C4 criteria.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report