search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

Dliteofficial - Missing check in `FlatcoinVault::setExecutabilityAge()` makes the time between executableAtTime and the maxExecutabilityAge short, resulting in more expired orders #148

Closed sherlock-admin2 closed 6 months ago

sherlock-admin2 commented 7 months ago

Dliteofficial

medium

Missing check in FlatcoinVault::setExecutabilityAge() makes the time between executableAtTime and the maxExecutabilityAge short, resulting in more expired orders

Summary

There is a missing check in FlatcoinVault::setExecutabilityAge() which makes it difficult for an order to be executed.

Vulnerability Detail

The setExecutabilityAge() is used to set the minimum and maximum order executability age. There is a check which ensures that none of the value provided is 0 but there isnt a check that prevents the minimum from being higher than the maximum. This missing check makes it possible to set the minimum and maximum to 60 seconds and 5 seconds respectively, for example.

Impact

Using the values above, this means that order can only be executed in a 5 seconds window, 60 seconds from when the order was announced. With a short order execution window, there'll be more expired orders than executed ones.

Code Snippet

FlatcoinVault::setExecutabilityAge()

    function setExecutabilityAge(uint64 _minExecutabilityAge, uint64 _maxExecutabilityAge) public onlyOwner {
        if (_minExecutabilityAge == 0) revert FlatcoinErrors.ZeroValue("minExecutabilityAge");
        if (_maxExecutabilityAge == 0) revert FlatcoinErrors.ZeroValue("maxExecutabilityAge");

        minExecutabilityAge = _minExecutabilityAge;
        maxExecutabilityAge = _maxExecutabilityAge;
    }

DelayedOrder::_prepareAnnouncementOrder()

    function _prepareAnnouncementOrder(uint256 keeperFee) internal returns (uint64 executableAtTime) {
        vault.settleFundingFees();

        if (keeperFee < IKeeperFee(vault.moduleAddress(FlatcoinModuleKeys._KEEPER_FEE_MODULE_KEY)).getKeeperFee())
            revert FlatcoinErrors.InvalidFee(keeperFee);

        cancelExistingOrder(msg.sender);

        executableAtTime = uint64(block.timestamp + vault.minExecutabilityAge());
    }

DelayedOrder::_prepareExecutionOrder()


    function _prepareExecutionOrder(address account, uint256 executableAtTime) internal {
        if (block.timestamp > executableAtTime + vault.maxExecutabilityAge()) revert FlatcoinErrors.OrderHasExpired();

        if (block.timestamp < executableAtTime) revert FlatcoinErrors.ExecutableTimeNotReached(executableAtTime);

        delete _announcedOrder[account];
    }

Tool used

Manual Review

Recommendation

    function setExecutabilityAge(uint64 _minExecutabilityAge, uint64 _maxExecutabilityAge) public onlyOwner {
        if (_minExecutabilityAge == 0) revert FlatcoinErrors.ZeroValue("minExecutabilityAge");
        if (_maxExecutabilityAge == 0) revert FlatcoinErrors.ZeroValue("maxExecutabilityAge");
+       if (_minExecutabilityAge >= _maxExecutabilityAge) revert;

        minExecutabilityAge = _minExecutabilityAge;
        maxExecutabilityAge = _maxExecutabilityAge;
    }
sherlock-admin commented 6 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

nevillehuang commented 6 months ago

Invalid, similar to #13, admin only function, they are trusted to set appropriate parameters. This is purely a sanity check