USAGE OF `block.number` WITH `ARBITRUM` AND `OPTIMISM` WILL BREAK THE CORE FUNCATIONALITY OF THE PROTOCOL

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L386 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L360 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ARCDVestingVault.sol#L106 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L15-L24 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L167-L170

Vulnerability details

Impact

The ArcadeTreasury._spend and ArcadeTreasury._approve functions are used to spend and approve tokens from the treasury respectively. Both the above mentioned functions use the block.number as the key value in the blockExpenditure mapping to store the amount of tokens spent within the current block. The blockExpenditure[block.number] value can not be greater than the limit which is passed into the function. This condition is enforced as follows:

    uint256 spentThisBlock = blockExpenditure[block.number];
    if (amount + spentThisBlock > limit) revert T_BlockSpendLimit(); 

The documentation of the protocol states that this protocol is implemented on EVM blockchains as show below:

Arcade.xyz is a platform for autonomous borrowing, lending, and escrow of NFT collateral on EVM blockchains.

Hence the protocol will be used in the EVM layer2 solutions such as optimism and arbitrum as well.

But the problem here is that on Optimism, the block.number is not a reliable source of timing information and the time between each block is also different from Ethereum. This is because each transaction is it's own block in optimism and blocks are not produced at a constant rate.

Hence the above conditional check on blockExpenditure[block.number] to cap the expenditure within a specific block will be deemed useless since each transaction has its own block.number. Hence the purpose of this check, which is to limit the amount of tokens spent from the treasury, within a certain block, is not properly implemented in optimism.

The same concern is applicable to arbitrum as well.

Similarly in the ARCDVestingVault.addGrantAndDelegate function, if the startTime input parameter is set to zero then the startTime will be made the block number which the tx is in, as shown below:

    if (startTime == 0) {
        startTime = uint128(block.number);
    }

And the subsequent timestamps such as the expiration and cliff are all using the block.number of respective blocks, as a measure of time. As it was explained earlier the block.number is not a reliable source of timing information on layer2 EVM chains such as optimism and arbitrum.

For an example when the user is using the same protocol in mainnet and optimism he will experience much shorter time durations with regard to startTime, cliff and expiration in optimism. This could break the core functionality of the protocol, create confusion and lower the user experience.

Similarly the ArcadeGSCCoreVoting contract is inheriting from the CoreVoting contract. The CoreVoting contract has calculated DAY_IN_BLOCKS value as 6496 by assuming the block time of 13.3 seconds. The subsequent vlaues lockDuration, extraVoteTime are calculated using the DAY_IN_BLOCKS value.

These values are used in conditinal checks of the critical function CoreVoting.proposal as shown below:

    require(
        lastCall > block.number + lockDuration + extraVoteTime,
        "expires before voting ends"
    );

This is to ensure that the expiration timestamp of the proposal is valid. But since we are using the DAY_IN_BLOCKS = 6496 to calculate the above timelines, it provides inaccurate time measurements when used with optimism and arbitrum. This is because the blocks are not produced at a constant rate in optimism and arbitrum. Hence this will break the core functionality of the CoreVoting.proposal function as far as the timestamps are concerned.

Proof of Concept

        uint256 spentThisBlock = blockExpenditure[block.number];

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L386

        uint256 spentThisBlock = blockExpenditure[block.number];

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L360

            startTime = uint128(block.number);

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ARCDVestingVault.sol#L106

    uint256 public constant DAY_IN_BLOCKS = 6496;

    // minimum time a proposal must be active for before executing
    // Default to 3 days, this avoids weekend surprise proposals
    uint256 public lockDuration = DAY_IN_BLOCKS * 3;

    // The number of blocks after the proposal is unlocked during which
    // voting can continue. Max vote time = lockDuration + extraVoteTime
    // Default to ~5 days of blocks, ie 8 days max vote time
    uint256 public extraVoteTime = DAY_IN_BLOCKS * 5;

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L15-L24

        require(
            lastCall > block.number + lockDuration + extraVoteTime,
            "expires before voting ends"
        );

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/external/council/CoreVoting.sol#L167-L170

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended to use epoch based implementation using block.timestamp to store the blockExpenditure details within a certain epoch duration, for more accurate measurement of time, instead of using block.number.

Hence it is recommended to consider using block.timestamp instead of block.number for more accurate measurement of time.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

PowVT marked the issue as sponsor acknowledged

[PowVT]

These contracts are only indented to be deployed on mainnet.

[0xean]

@PowVT - that may be the case, but it seems it was not clearly documented. I am inclined to award this as M as the warden analyzed the code given the documentation presented and found a valid issue if this code was to be deployed to "EVM blockchains" in general.

thoughts?

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[PowVT]

ok I see, yeah we do say "EVM Chains". But seems more like a QA thing to us.

[0xean]

Considering the sponsors stated in discord this was for mainnet only, I am going to mark this as QA. Its hard to be fair to all wardens in this situation given that some may not have seen the discord message.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[0xean]

bumping the warden QA report to "A"

[c4-judge]

0xean marked the issue as grade-a

[udsene]

Hi @0xean, The issue #312 was reported based on the information given in the documentation. Documentation mentions that Arcade.xyz is operated on EVM blockchains in general. But it seems the issue has been downgraded based on details provided in the discord which are not conclusive enough due to following reasons.

1. The documentation explicitly mentions that Arcade.xyz is operated on EVM blockchains in general.
2. Wardens audit the code bases based on the official documentation provided and uses discord to clarify unclear areas of the code base.
3. Since many queries are made in the discord there is a possibility a warden might miss out on certain information given in the discord (which was the case here).
4. With regard to the issue #312, even the information given in the discord is not conclusive enough and mentions the following: They will be deployed on Mainnet.
5. It dosn't clearly state that contracts will be deployed on Mainnet only.
6. Niether does it say the contracts will not be deployed in other blockchains.
7. Hence the provided information in the discord regarding the deployed blockchains is not conclusive enough to deny the information explicitly given in the documentation which states that Arcade.xyz is operated in EVM blockchains in general.
8. Even for a warden who sees the information in the discord there is a high probability to assume that initial deployment of the contracts will be on the mainnet and in the future, contracts will be deployed in other EVM blockchains since it is explicitly mentioned in the documentation.

Hence based on the above provided reasons I would like to kindly request to reconsider the severity of this issue and consider escalating it to a medium since the warden has submitted the bug based on the information provided to him.

Note : If in the documentation the EVM blockchains was not mentioned explicitly, then i agree that the provided information in the discord is enough to conclude that the contracts will be deployed on the mainnet only. But since in the documenation it explicitly states EVM blockchains and the given information in the discord does not conclusively deny this fact, I would kindly request to reconsider the severity of the reported issue #312.

As a humble request, I would like to suggest that the official documentation provided, should be given the priority in future contests and if there is a discrepency between the documentation and the actual implementation it should be clearly mentioned atleast as a pinned message in the discord so every warden is clearly alerted of the change. This will save both time and effort of all the stake holders involved in the contest and negate any confusion. If above is not followed then the wardens should be given the benefit of the doubt since they invest lot of time and effort into reporting bugs based on the provided information.

[DadeKuma]

This issue is invalid.

I would like to add that this is a generic, common issue that is usually reported by bots. It wasn't reported in the automated findings, as the contracts are deployed only on mainnet.

Also note that:

• Existing Arcade.xyz contracts are deployed only on mainnet
• The README says: - Does it use a side-chain?: False
• The sponsor explicitly said that is deployed only on mainnet

[udsene]

Please consider the following comments:

1. Existing Arcade.xyz contracts are deployed only on mainnet
   • The documentation says Arcade.xyz is a platform for autonomous borrowing, lending, and escrow of NFT collateral on EVM blockchains. which creates possibility of future deployment into other EVM blockchains.
2. The README says: - Does it use a side-chain?: False
   • I believe the Arbitrum and Optimism are not considered side-chains and are Layer 2 roll-up solutions (Willing to be corrected if i am wrong).
3. The sponsor explicitly said that is deployed only on mainnet
   • The exact wording given in the discord is They will be deployed on Mainnet which does not include the only mainnet part and as a result does not explicitly deny the statement given in the documentation, as explained in my previous comment.

I will accept the final judgment which will be given by the judge on this regard without further dispute.

[JeffCX]

Hello

This is a great report but I would politely side with @0xean judge

there is insufficient evidence to support this claim and assumption in the original report

Hence the protocol will be used in the EVM layer2 solutions such as optimism and arbitrum as well.

https://docs.arcade.xyz/docs/architecture

The Arcade Protocol is a set of smart contracts deployed on the Ethereum blockchain that facilitates trustless peer to peer borrowing, lending, and escrow of NFT assets.

and sponsor confirm the "only mainnet" deployment.

[0xean]

I am going to leave this as graded.

As more and more EVM based chains launch, there is going to be more potential issues with writing solidity that is generic enough for them all (hooks, block timing, etc).

I do think c4 should do a better job in screening contests so its explicit which chains are in or out of scope and will ask the team if that can be added to the standard list of questions in a way that is more clear (IE not side chains vs L1s L2s, etc).

I think it should read "Which blockchains will this code be deployed to and are considered in scope for this audit?"

There is no perfectly fair outcome here and for that all we can do is make the process better next time.