_checkDelay will not work properly for Arbitrum or Optimism due to block.number

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L857-L868

Vulnerability details

Impact

Trade delay will not work correctly on Arbitrum allowing users to exploit multiple valid prices

Proof of Concept

function _checkDelay(uint _id, bool _type) internal {
    unchecked {
        Delay memory _delay = blockDelayPassed[_id];
        //in those situations
        if (_delay.actionType == _type) {
            blockDelayPassed[_id].delay = block.number + blockDelay;
        } else {
            if (block.number < _delay.delay) revert("0"); //Wait
            blockDelayPassed[_id].delay = block.number + blockDelay;
            blockDelayPassed[_id].actionType = _type;
        }
    }
}

_checkDelay enforces a delay of a specific number of block between opening and closing a position. While this structure will work on mainnet, it is problematic for use on Arbitrum. According to Arbitrum Docs block.number returns the most recently synced L1 block number. Once per minute the block number in the Sequencer is synced to the actual L1 block number. This period could be abused to completely bypass this protection. The user would open their position 1 Arbitrum block before the sync happens, the close it the very next block. It would appear that there has been 5 block (60 / 12) since the last transaction but in reality it has only been 1 Arbitrum block. Given that Arbitrum has 2 seconds blocks I would be impossible to block this behavior through parameter changes.

It also presents an issue for Optimism because each transaction is it's own block. No matter what value is used for the block delay, the user can pad enough tiny transactions to allow them to close the trade immediately.

Tools Used

Manual Review

Recommended Mitigation Steps

The delay should be measured using block.timestamp rather than block.number

[TriHaz]

Once per minute the block number in the Sequencer is synced to the actual L1 block number.

That is changed after Nitro upgrade.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

@TriHaz I'd like to flag this issue with the following notes:
block.number will return the latest synched block number from L1, this can be stale

Per the docs:

As a general rule, any timing assumptions a contract makes about block numbers and timestamps should be considered generally reliable in the longer term (i.e., on the order of at least several hours) but unreliable in the shorter term (minutes). (It so happens these are generally the same assumptions one should operate under when using block numbers directly on Ethereum!)

From a trusted Arbitrum Dev:
using block.number is generally fine if you want to measure time, since that will roughly follow L1 block time

So ultimately this is dependent on how big or small of a delay is required.

For minutes to hours, there seems to be no risk, while for shorter timeframes, some risk is possible.

In terms of impact, the main impact would be that a operation that would be expected to be executed 12 seconds later, could actually be executed as rapidly as 1 or 2 seconds after (if we assume that one L2 block goes from number A to B)

I don't think the finding can be categorized High Severity due to the reliance on settings and intentions, but at this point I believe the finding is valid and am thinking it should be of Medium Severity as it may break expectations (e.g. being able to use the same oracle price in 2 separate blocks due to unexpectedly small timestamp differences), but this is reliant on an external condition

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

I have also recently checked Optimism Docs, in anticipation of the Bedrock upgrade.

Very notable warning

Source: https://community.optimism.io/docs/developers/bedrock/how-is-bedrock-different/

Leading me to further agree with the risk involved with the finding, at this time I believe block.timestamp to be a better tool for all L2 integrations

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GainsGoblin]

Mitigation: code-423n4/2022-12-tigris#2 (comment)

[c4-sponsor]

GainsGoblin marked the issue as sponsor confirmed