block.timestamp dependence of cross-chain calls can skip accountHumanity update

[hats-bug-reporter[bot]]

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

Description: Description\ When humanity is being transferred to a foreign chain, expirationTime is being sent as part of the payload to later determine whether humanity is claimed or not in ProofOfHumanity::ccGrantHumanity:

function ccGrantHumanity(
        bytes20 _humanityId,
        address _account,
        uint40 _expirationTime
    ) external onlyCrossChain returns (bool success) {
        Humanity storage humanity = humanityData[_humanityId];

        // If humanity is claimed, don't overwrite.
        if (humanity.owner != address(0x0) && block.timestamp < humanity.expirationTime) return false;

        // Must not be in the process of claiming a humanity.
        require(humanityData[accountHumanity[_account]].requestCount[_account] == 0);

        humanity.owner = _account;
        humanity.expirationTime = _expirationTime;
        accountHumanity[_account] = _humanityId;

        emit HumanityGrantedDirectly(_humanityId, _account, _expirationTime);

        return true;
    }

But on different chains block.timestamp means different things:

According to the Arbitrum doc:

Timestamp boundaries of the sequencer

As mentioned, block timestamps are usually set based on the sequencer's clock. Because there's a possibility that the sequencer fails to post batches on the parent chain (for example, Ethereum) for a period of time, it should have the ability to slightly adjust the timestamp of the block to account for those delays and prevent any potential reorganisations of the chain. To limit the degree to which the sequencer can adjust timestamps, some boundaries are set, currently to 24 hours earlier than the current time, and 1 hour in the future.

https://docs.arbitrum.io/build-decentralized-apps/arbitrum-vs-ethereum/block-numbers-and-time#block-timestamps-arbitrum-vs-ethereum

Block timestamps on Arbitrum are not linked to the timestamp of the L1 block. They are updated every L2 block based on the sequencer's clock. These timestamps must follow these two rules:

Must be always equal or greater than the previous L2 block timestamp

Must fall within the established boundaries (24 hours earlier than the current time or 1 hour in the future). More on this below.

Furthermore, for transactions that are force-included from L1 (bypassing the sequencer), the block timestamp will be equal to either the L1 timestamp when the transaction was put in the delayed inbox on L1 (not when it was force-included), or the L2 timestamp of the previous L2 block, whichever of the two timestamps is greater.

Attack Scenario\

Knowing that we can say there is a block.timestamp dependence of the 2 chains - originator and foreign and when CrossChainProofOfHumanity::transferHumanity (function that sends the payload to the foreign chain and ends calling the CrossChainProofOfHumanity::receiveTransfer) is called towards the end of the claiming period there can be the scenario when block.timestamp >= humanity.expirationTime (humanity is not claimed) on the originator chain, but on the foreign for that particular humanity to be block.timestamp < humanity.expirationTime (false indication of claimed humanity) and to not update the other params of the humanity struct:

function receiveTransfer(
        address _owner,
        bytes20 _humanityId,
        uint40 _expirationTime,
        bytes32 _transferHash
    ) external override allowedGateway(msg.sender) {
        // Once transfer hash is flagged as received it is not possible to receive the transfer again
        require(!receivedTransferHashes[_transferHash]);

        // If humanity is claimed on the main contract it will return false and not override the state
        // Otherwise requires _owner to not be in process of claiming a humanity
        bool success = proofOfHumanity.ccGrantHumanity(_humanityId, _owner, _expirationTime); //@audit this one

        CrossChainHumanity storage humanity = humanityData[_humanityId];

        // Clear human humanityId for past owner
        delete accountHumanity[humanity.owner];

        // Overriding this data in case it is outdated
        if (success) {
            accountHumanity[_owner] = _humanityId;

            humanity.owner = _owner;
            humanity.expirationTime = _expirationTime;
            humanity.isHomeChain = true;
            humanity.lastTransferTime = uint40(block.timestamp);
        }

        receivedTransferHashes[_transferHash] = true;

        emit TransferReceived(_humanityId, _owner, _expirationTime, _transferHash);
    }

Attachments

1. Proof of Concept (PoC) File
2. Revised Code File (Optional) Do not rely on cross-chain block.timestamp dependencies,

[clesaege]

Ok, so my understanding of your concern is that:

• The humanity is claimed on both platforms. With different expiration times. Let's say ExpA and ExpB.
• There is a timestamp missmatch between those 2. Let's say TimestampA < TimestampB
• Such that we have TimestampA < ExpB < TimestampB.
• The user thinking that the profile is expired on B (due to the timestamp he sees on A), wants to transfer his profile from A to B.
• When the requests reach B. The humanity is not transferred as the humanity currently on B is still considered valid.

So this cannot happen because a profile cannot be registered on both chains as this would be a duplicate (and here the user losing one of the profile would actually be something which is desirable, not a bug).

As per contest rule, the following are out of scope:

• A user losing his profile if someone else managed to register a profile with the same id on the target platform (this is not prevented by code, but by challenging duplicate profiles even through different platforms). (well here it would be the same person, but the comments do explain that it should be challenged)
• Issues about challengers missing some invalid profile submissions/removals (for the purpose of this review, we will assume challengers are perfect and omniscient and that they always challenge invalid actions before the deadline).

And even if it was in scope, that would actually solve an invalid situation to a valid one.