mrpathfindr - Dangerous arbitrary timestamp parameters may lead to inaccurate results.

[sherlock-admin]

mrpathfindr

medium

Dangerous arbitrary timestamp parameters may lead to inaccurate results.

Summary

The functions putPrice and updatePrices controlled by the FEEDER_ROLE contain arbitrary timestamp values. These two functions are responsible for setting the current price or prices of an asset. As such the timestamp at which they are updates should be the current timestamp and nothing else.

Vulnerability Detail

Let us examine the function putPrice

    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"); //@audit update the price to block.timestamp
        prices[asset] = IOracle.Price(asset, timestamp, prev_timestamp, price, prev_price);
        emit newPrice(asset, timestamp, price);
    }
    

The timestamp can be set to a time in the future, e.g (block.timestamp + 3 days ) which would be the incorrect putTimeStamp.

Impact

It is possible for the FEEDER to set a timestamp that is unrealistic or inaccurate rather than the correct timestamp.

Code Snippet

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

Tool used

Manual Review

Recommendation

I recommend mitigating the potential for an incorrect timestamp being applied by setting the value to the current time at which the function is called. Like so:

  function putPrice(address asset, uint256 price) public onlyRole(FEEDER_ROLE) {
        uint64 prev_timestamp = prices[asset].timestamp;
        uint256 prev_price = prices[asset].price;

        prices[asset] = IOracle.Price(asset, block.timeStamp, prev_timestamp, price, prev_price);
        emit newPrice(asset, timestamp, price);
    }

Additionally. with this implementation, there will be no need for this statement require(prev_timestamp < block.timeStamp, "Outdated timestamp"); as block.timeStamp will always be greater than prev_timestamp `

[mrpathfindr]

Escalate for 10 USDC
This should be a valid medium as it is not the expected behaviour for time at which the price for an asset has been updated to be an arbitrary value that could be any value in the future.

The put price should be block.timeStamp. The FEEDER should not be able to manipulate the timestamp.

This block of code require(prev_timestamp < timestamp, "Outdated timestamp"); is completely redundant.

The current implementation leaves the protocol exposed to timestamp manipulation that can be easily avoided if at the time putPrice is called by the feeder, timestamp in IOracle.Price is set to block.timeStamp

Impact: Let's say there are multiple feeder for each stable currency, one feeder updates the put price to 10 years in the future, a subsequent feeder will have to update the price to a time above 10 years. An obvious issue for the protocol.

[sherlock-admin]

Escalate for 10 USDC
This should be a valid medium as it is not the expected behaviour for time at which the price for an asset has been updated to be an arbitrary value that could be any value in the future.

The put price should be block.timeStamp. The FEEDER should not be able to manipulate the timestamp.

This block of code require(prev_timestamp < timestamp, "Outdated timestamp"); is completely redundant.

The current implementation leaves the protocol exposed to timestamp manipulation that can be easily avoided if at the time putPrice is called by the feeder, timestamp in IOracle.Price is set to block.timeStamp

Impact: Let's say there are multiple feeder for each stable currency, one feeder updates the put price to 10 years in the future, a subsequent feeder will have to update the price to a time above 10 years. An obvious issue for the protocol.

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.

[ctf-sec]

admin input issue is not valid

[0xruhum]

protocol doesn't really care about the timestamp anyways. It always gets the most recent entry

[jacksanford1]

Should remain invalid due to the assumption that an admin will make a mistake:

It is possible for the FEEDER to set a timestamp that is unrealistic or inaccurate rather than the correct timestamp.

[hrishibhat]

Result:
Low
Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• mrpathfindr: rejected