sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

BengalCatBalu - Broken Assumtion of timeFrame behaviour #317

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

BengalCatBalu

medium

Broken Assumtion of timeFrame behaviour

Summary

As far as I understand from personal correspondence with the protocol team, the following is assumed to be invariant Work.opensAt < work.closesAt. However, the protocol does not validate this in any way when creating a work, nor when changing the timeFrame in the Edition.sol::setTimeFrame function

Thus, the invariant can easily be violated by the user, intentionally or unintentionally

Vulnerability Detail

When a new job is created or the creator wants to change the timeFrame, work.opensAt and work.closesAt are not validated in any way

Creating a job

function publish(
        address creator_,
        uint256 maxSupply_,
        uint64 opensAt_,
        uint64 closesAt_,
        Node[] calldata attributions_,
        Strategy calldata strategy_,
        Metadata calldata metadata_
    ) external override onlyRoles(EDITION_MANAGER_ROLE) returns (uint256 tokenId) {
        tokenId = ++totalWorks; // first increments, then assigns

        _metadata[tokenId] = metadata_;
        works[tokenId] = Work({
            creator: creator_,
            totalSupply: 0,
            maxSupply: maxSupply_,
            opensAt: opensAt_,
            closesAt: closesAt_,
            strategy: FEE_MANAGER.validateStrategy(strategy_)
        });
...

timeFrame change

function setTimeframe(uint256 tokenId, uint64 opensAt, uint64 closesAt) external {
        Work storage work = works[tokenId];
        if (msg.sender != work.creator) revert Unauthorised();

        // Update the open and close times for the work
        work.opensAt = opensAt;
        work.closesAt = closesAt;

        emit TimeframeUpdated(address(this), tokenId, opensAt, closesAt);
    }

Impact

Since the Readme says to report broken invariants, I included it in the bug, especially in combination with the issue described here Title: The work creator can manipulate FeeSize using front-running, it gives even more power to work.creator

For example, it can easily turn off mint for a certain interval, just by changing the timeFrame once, making opensAt > closesAt, checkTime will revert the transaction exactly at the interval (closesAt, opensAt)

function _checkTime(uint64 start_, uint64 end_) internal view {
        if (block.timestamp < start_ || (end_ != 0 && block.timestamp > end_)) {
            revert NotOpen(start_, end_);
        }
    }

Code Snippet

Tool used

Manual Review

Recommendation

Check the invariant when assigning time