Lack of Input Validation can Permanently DOS Oracle

Summary

In XOracle.sol, the prices for each asset are updated via the putPrice() function from the FEEDER_ROLE. The input price and timestamp overwrite the existing data in the prices mapping for the particular assets. The function contains a check that the existing prices[asset].timestamp is less than the input timestamp, otherwise it reverts. This creates an interesting possibility for the contract to DOS itself if an input timestamp is accidently set too far in the future. The FEEDER will not be able to update the price again until the existing timestamp has passed the current block.timestamp.

It is possible for the FEEDER to simply update the price with an even larger input timestamp, but that would require inputting an even more incorrect timestamp.

Vulnerability Detail

The putPrice() function only allows updating an asset price/timestamp if the input timestamp is less than the asset's timestamp in storage. There is no validation that the input timestamp is less than or equal to block.timestamp to protect against inputting a timestamp too far in the future.

Impact

Inability to update asset prices without further adding to the incorrect future timestamp discrepancy.

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/XOracle.sol#L26-L32

Tool used

Manual Review

Recommendation

Add another check that the input timestamp is less than or equal to block.timestamp.

    function putPrice(address asset, uint64 timestamp, uint256 price) public onlyRole(FEEDER_ROLE) {
        uint64 prev_timestamp = prices[asset].timestamp;
        uint256 prev_price = prices[asset].price;
        require(prev_timestamp < timestamp, "Outdated timestamp");
++      require(timestamp <= block.timestamp, "No future timestamps");
        prices[asset] = IOracle.Price(asset, timestamp, prev_timestamp, price, prev_price);
        emit newPrice(asset, timestamp, price);
    }

fatherGoose1 commented 1 year ago

Escalate for 10 USDC This submission outlines a valid lack of input check that can lead to DOS or degraded functionality. A single errored input of the unbounded timestamp parameter will cause either of the following to occur.

1) Protocol can not function correctly due to lack of ability to update the price (due to require statement require(prev_timestamp < timestamp, "Outdated timestamp");) 2) Protocol must continue using a timestamp that is larger than the actual current timestamp in order to update the prices.

A simple fix is to ensure that the FEEDER can not falsely input a price with timestamp that's greater than block.timestamp. Or simply remove the timestamp parameter and use block.timestamp instead. Requiring an input timestamp that is boundless to the positive end leaves room for misuse.

sherlock-admin commented 1 year ago

Escalate for 10 USDC This submission outlines a valid lack of input check that can lead to DOS or degraded functionality. A single errored input of the unbounded timestamp parameter will cause either of the following to occur.

1) Protocol can not function correctly due to lack of ability to update the price (due to require statement require(prev_timestamp < timestamp, "Outdated timestamp");) 2) Protocol must continue using a timestamp that is larger than the actual current timestamp in order to update the prices.

A simple fix is to ensure that the FEEDER can not falsely input a price with timestamp that's greater than block.timestamp. Or simply remove the timestamp parameter and use block.timestamp instead. Requiring an input timestamp that is boundless to the positive end leaves room for misuse.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

0xruhum commented 1 year ago

Admin input validation is not a valid issue in Sherlock contests. As you say yourself, you can just continue using the incorrect timestamps. The protocol doesn't really care about the timestamp anyways. It always uses the latest entry. In the worst case, you deploy a new oracle through the timelock

ctf-sec commented 1 year ago

Yes, Admin input validation is not a valid issue in Sherlock contests, this requires the admin to make a mistake and input timestamp that are far from the current timestamp, a valid low / informational finding