Wrong block will be verified for snapshot

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xac8c071250c3ad10c7d4cb1db6be15c7b400705f7450a1c23d97366783e3475d Severity: medium

Description: Description\

The issue of verification belongs to Arbitrum and Optimism chain as they have different pattern for block number as mentioned in thier documentation.

Accessing block number within a Arbitrum chain will return value synced to the L1 block number which the sequencer has received the transaction.

In Optimism the block.number is not a reliable source of time based information and the time between each block is also different from Mainnet.

This is because each transaction on Optimism is placed in a separate block and blocks are not produce at a constant rate.

Attack Scenario\

Using block.number as a time measurement standard can lead to inaccurate verification of snapshot taken for a block.

The difference can grow significantly as the verification of snapshot will be attached to previous result using latestRoot.rootHash in L#104 of Ledger.sol.

Attachments https://github.com/hoprnet/hoprnet/blob/master/packages/ethereum/contracts/src/Ledger.sol#L102

1. Proof of Concept (PoC) File

The indexEvent() function will not be able to detect and verify correct block in which snapshots were taken due to use of block.number that generate different result on different L2 chains.

    function indexEvent(bytes memory payload) internal {
        bool createSnapshot = false;
        if (block.timestamp > latestRoot.timestamp + snapshotInterval) {
            createSnapshot = true;
        }

        // take first 28 bytes
        latestRoot.rootHash = bytes28(
            keccak256(
                // keep hashed data minimal
                abi.encodePacked(
                    // ledger feed must be unique
                    ledgerDomainSeparator,
                    // Allows the verifier to detect up until which block the snapshot includes state changes
                    uint32(block.number),
                    // Bind result to previous root
                    latestRoot.rootHash,
                    // Information about the happened state change
                    keccak256(payload)
                )
            )
        );
        latestRoot.timestamp = uint32(block.timestamp);

        if (createSnapshot) {
            latestSnapshotRoot = latestRoot;
        }
    }

2. Revised Code File (Optional)

Use block.timestamp rather than block.number to have accurate verification of blocks in which snapshot was taken.

    function indexEvent(bytes memory payload) internal {
        bool createSnapshot = false;
        if (block.timestamp > latestRoot.timestamp + snapshotInterval) {
            createSnapshot = true;
        }

        // take first 28 bytes
        latestRoot.rootHash = bytes28(
            keccak256(
                // keep hashed data minimal
                abi.encodePacked(
                    // ledger feed must be unique
                    ledgerDomainSeparator,
                    // Allows the verifier to detect up until which block the snapshot includes state changes
-                 uint32(block.number),
+               uint32(block.timestamp),
                    // Bind result to previous root
                    latestRoot.rootHash,
                    // Information about the happened state change
                    keccak256(payload)
                )
            )
        );
        latestRoot.timestamp = uint32(block.timestamp);

        if (createSnapshot) {
            latestSnapshotRoot = latestRoot;
        }
    }

[QYuQianchen]

Contracts are used on Gnosis chain

[Abbasjafri-syed]

As per Hopr's docs, the protocol is designed to be compatible with various EVM chains, as it was previously first deployed on mainnet.

There exist a significant chance that in near future it will be deployed on above mentioned and other networks for the same reason or due to growth and demand of the protocol. The usage of block.number , will then create problem in smooth functioning of the protocol.

[QYuQianchen]

Thanks for providing the info. This design does not rely on the uniqueness of block.number Even on L1, this design works when multiple indexEvent function get called inside one the same block.

[Abbasjafri-syed]

If indexEvent function is called multiple times in a same block...wouldn't it be dependent on block.number.

This will result in taking a snapshot for all multiple calls to have a same block.number as all state changes carried before will be recorded through the same block.

[QYuQianchen]

If indexEvent gets called multiple times in a same block, the rootHash gets updated each time. It's desired because the purpose of the ledge is to index all the events and create a linked list, so that an external party can verify the root hash with a full history given.