Block.timestamp check in Delayorder:_prepareExecutionOrder is wrong

Summary

block.timestamp checks to see if order has expired however the check does extend the expiration time by length of minExecutabilityAge

Vulnerability Detail

In flatcoinVault.sol the following parameter were defined

/// @notice The minimum time that needs to expire between trade announcement and execution.
    uint64 public minExecutabilityAge;

    /// @notice The maximum amount of time that can expire between trade announcement and execution.
    uint64 public maxExecutabilityAge;

thus trade is valid between (AnnouncementTime +maxExecutabilityAge) - (AnnouncementTime +minExecutabilityAge);

in Delayorder:_prepareExecutionOrder

if (block.timestamp > executableAtTime + vault.maxExecutabilityAge()) revert FlatcoinErrors.OrderHasExpired();
//this is used to check if trade has expired

//However, 
executableAtTime  =uint64(block.timestamp + vault.minExecutabilityAge());

//While the maxExecutabilityAge is defined as the maximum time allowable between trade announcement and execution, the IF statement incorrectly checks if block.timestamp is greater than the time from trade announcement to execution (maxExecutabilityAge). Additionally, it erroneously adds the minimum time required between trade announcement and execution (minExecutabilityAge).

Impact

This will extend trade execution period by the length of minExecutabilityAge

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L651

Tool used

Manual Review

Recommendation

To check Trade validity period check if Block.stamp >maxExecutabilityAge

Duplicate of #165

sherlock-admin commented 7 months ago

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

takarez commented:

invalid

Afriauditor commented 7 months ago

This is a duplicate of issue #165 and the reason for invalidation by judge is "Invalid, intended functionality to only count executability time expiration from end of minimum time onwards." however this is wrong because it was stated multiple times in the code that intended functionality is to count from time of trade announcement. see https://github.com/sherlock-audit/2023-12-flatmoney-Afriauditor/blob/f1915c149283eeb11f125d5b6d43fc11a2e4b1da/flatcoin-v1/src/FlatcoinVault.sol#L33 and https://github.com/sherlock-audit/2023-12-flatmoney-Afriauditor/blob/f1915c149283eeb11f125d5b6d43fc11a2e4b1da/flatcoin-v1/src/FlatcoinVault.sol#L416 . Time is expected to expire between trade announcement and execution which is minexecutabilityage. Judge wrongfully assumes that Maxexecuitabilityage counts from point of execution i.e minexecuitabilityage which is wrong. It also begins counting from point of trade announcement just like Minexecuitabilityage thereby lenghtning trade execuitability period as clearly stated in the link above. @rashtrakoff pls kindly have a look

rashtrakoff commented 7 months ago

That's the intended functionality. maxExecutabilityAge from executableAtTime which basically means order will expire when time is greater than announcedTime+minExecutabilityAge+maxExecutabilityAge.