Missing check for validity of timestamp

Summary

The absence of a validity check for the message timestamp in the Yolo contract may allow the repeated use of an NFT floor price from the past, potentially compromising the games.

Vulnerability Detail

In the Yolo contract, when receiving NFT floor price data from the Reservoir oracle, the contract verifies whether the signature's timestamp is stale. The current implementation checks if the block.timestamp exceeds the floorPrice.timestamp by a set validity period.

    function _verifyReservoirSignature(address collection, ReservoirOracleFloorPrice calldata floorPrice) private view {
        if (block.timestamp > floorPrice.timestamp + uint256(signatureValidityPeriod)) {
            revert SignatureExpired();
        }

https://github.com/sherlock-audit/2024-01-looksrare/blob/314fd5b753359e07778724342de8dda20713ec2e/contracts-yolo/contracts/YoloV2.sol#L1600-L1603

However, it lacks a crucial check to ensure that the floorPrice.timestamp is not in the future (if message.timestamp <= block.timestamp).

We can see the correct checking in the code provided by Reservoir:

    // Ensure the message timestamp is valid
    if (
        message.timestamp > block.timestamp ||
        message.timestamp + validFor < block.timestamp
    ) {
        return false;
    }

https://github.com/reservoirprotocol/oracle/blob/main/contracts/ReservoirOracle.sol#L49-L55

In known issue, the floor prices are expected to be trusted, but not the message.timestamp. The timestamp should be sufficiently verified.

Impact

In case the Reservoir Oracle provides an error timestamp far in the future, the price value can be used over and over again with the wrong price until the future time becomes less than block.timestamp - validFor. Wrong price is a serious issue since it is used to price the NFT collections in the Yolo game.

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/314fd5b753359e07778724342de8dda20713ec2e/contracts-yolo/contracts/YoloV2.sol#L1600-L1603

Tool used

Manual Review

Recommendation

Implement an additional check for the validity of message.timestamp:

-            if (block.timestamp > floorPrice.timestamp + uint256(signatureValidityPeriod)) {
+            if (block.timestamp > floorPrice.timestamp + uint256(signatureValidityPeriod)) || floorPrice.timestamp > block.timestamp {
                revert SignatureExpired();
            }

jpopxfile commented 9 months ago

https://github.com/LooksRare/contracts-yolo/pull/189

nevillehuang commented 9 months ago

@Czar102 This seems to be invalid based on sherlock rules for trusted external admins given it assumes that the price is signed with a future time stamp, which would be highly unlikely by reservoir. What do you think?

nevillehuang commented 9 months ago

request poc

An additional poc is not required, but I want to facilitate discussions between sponsor @0xhiroshi @jpopxfile and the watson

sherlock-admin commented 9 months ago

PoC requested from @thangtranth

Requests remaining: 4

thangtranth commented 9 months ago

The validation is implemented by the Reservoir to ensure that the timestamp is valid and safe to use. I make no assumption here.

Instead of inheriting from the Reservoir contract, Looksrare chooses to reimplement it, but missing the validation. I believe the issue is valid.

Czar102 commented 9 months ago

the floor prices are expected to be trusted, but not the message.timestamp

If the message.timestamp wasn't trusted, even with the existing fix this would be an issue since the oracle could sign for a timestamp in the future and you could use it then. I think it is clear that since the price can be determined at a certain time, both price and message.timestamp are trusted. Hence, I believe this is invalid.

thangtranth commented 9 months ago

Hi, So in this case, do you mean that it is unnecessary to the protocol to validate the message.timestamp because we assume that it is always true?