Inconsistent State in `CrossChainProofOfHumanity` After Failed Transfer

[hats-bug-reporter[bot]]

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

Description:

Inconsistent State in CrossChainProofOfHumanity After Failed Transfer

Description

The CrossChainProofOfHumanity contract contains a vulnerability in its receiveTransfer function. When a transfer fails (i.e., the humanity is already claimed), the function still updates certain state variables, leading to an inconsistent state. This inconsistency could potentially be exploited or cause unexpected behavior in the contract.

Issue Scenario

1. A valid transfer is received for a humanity, setting the owner and updating relevant mappings.
2. A second transfer is attempted for the same humanity with a different owner.
3. The second transfer fails because the humanity is already claimed.
4. Despite the failure, the contract clears the accountHumanity mapping for the original owner, breaking the link between the owner and their humanity.
5. The receivedTransferHashes mapping is updated, marking the failed transfer as received.

This scenario results in a state where:

• The original owner still owns the humanity in the humanityData mapping.
• The accountHumanity mapping no longer associates the original owner with their humanity.
• A transfer hash for a failed transfer is marked as received.

Attachments

1. Proof of Concept showcase the relevant issue code here

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

import "forge-std/Test.sol";
import "../contracts/mocks/MockProofOfHumanity.sol";
import "../contracts/mocks/MockCrossChainProofOfHumanity.sol";

contract CrossChainProofOfHumanityTest is Test {
    MockCrossChainProofOfHumanity public ccpoh;
    MockProofOfHumanity public poh;

    function setUp() public {
        poh = new MockProofOfHumanity();
        ccpoh = new MockCrossChainProofOfHumanity(address(poh));
    }

    function testInconsistentStateAfterFailedTransfer() public {
        bytes20 humanityId = bytes20(uint160(1));
        address owner1 = address(0x1);
        address owner2 = address(0x2);
        uint40 expirationTime = uint40(block.timestamp + 365 days);
        bytes32 transferHash1 = keccak256(abi.encodePacked(humanityId, owner1, expirationTime));
        bytes32 transferHash2 = keccak256(abi.encodePacked(humanityId, owner2, expirationTime));

        // First transfer succeeds
        ccpoh.receiveTransfer(owner1, humanityId, expirationTime, transferHash1);

        // Verify initial state
        (address storedOwner,,,) = ccpoh.humanityData(humanityId);
        assertEq(storedOwner, owner1);
        assertEq(ccpoh.accountHumanity(owner1), humanityId);

        // Second transfer fails (humanity already claimed)
        ccpoh.receiveTransfer(owner2, humanityId, expirationTime, transferHash2);

        // Verify final state
        (storedOwner,,,) = ccpoh.humanityData(humanityId);
        assertEq(storedOwner, owner1, "Humanity owner should still be owner1");
        assertTrue(ccpoh.receivedTransferHashes(transferHash2), "Transfer hash should be marked as received");
        assertEq(ccpoh.accountHumanity(owner1), bytes20(0), "owner1's accountHumanity should be cleared");
        assertEq(ccpoh.accountHumanity(owner2), bytes20(0), "owner2's accountHumanity should remain empty");
    }
}

Test Output

[PASS] testInconsistentStateAfterFailedTransfer() (gas: 157092)
Traces:
  [157092] CrossChainProofOfHumanityTest::testInconsistentStateAfterFailedTransfer()
    ├─ [126731] MockCrossChainProofOfHumanity::receiveTransfer(0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000001000000000000000000000000, 31536001 [3.153e7], 0x3859251ddf83c5d97dbb12410569eb983e1d065b893e202bb58437daa1195d20)
    │   ├─ [49571] MockProofOfHumanity::ccGrantHumanity(0x0000000000000000000000000000000000000001000000000000000000000000, 0x0000000000000000000000000000000000000001, 31536001 [3.153e7])
    │   │   ├─ emit HumanityGrantedDirectly(humanityId: 0x0000000000000000000000000000000000000001000000000000000000000000, owner: 0x0000000000000000000000000000000000000001, expirationTime: 31536001 [3.153e7])
    │   │   └─ ← [Return] true
    │   ├─ emit TransferReceived(humanityId: 0x0000000000000000000000000000000000000001000000000000000000000000, owner: 0x0000000000000000000000000000000000000001, expirationTime: 31536001 [3.153e7], transferHash: 0x3859251ddf83c5d97dbb12410569eb983e1d065b893e202bb58437daa1195d20)
    │   └─ ← [Stop]
    ├─ [721] MockCrossChainProofOfHumanity::humanityData(0x0000000000000000000000000000000000000001000000000000000000000000) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000001, 31536001 [3.153e7], 1, true
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return]
    ├─ [615] MockCrossChainProofOfHumanity::accountHumanity(0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000001000000000000000000000000
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000001000000000000000000000000, 0x0000000000000000000000000000000000000001000000000000000000000000) [staticcall]
    │   └─ ← [Return]
    ├─ [27181] MockCrossChainProofOfHumanity::receiveTransfer(0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000001000000000000000000000000, 31536001 [3.153e7], 0xf54e1cc4bb11dbb94484eb2de22518a87e125aa4a0ddf87679548e25a9edf64c)
    │   ├─ [994] MockProofOfHumanity::ccGrantHumanity(0x0000000000000000000000000000000000000001000000000000000000000000, 0x0000000000000000000000000000000000000002, 31536001 [3.153e7])
    │   │   └─ ← [Return] false
    │   ├─ emit TransferReceived(humanityId: 0x0000000000000000000000000000000000000001000000000000000000000000, owner: 0x0000000000000000000000000000000000000002, expirationTime: 31536001 [3.153e7], transferHash: 0xf54e1cc4bb11dbb94484eb2de22518a87e125aa4a0ddf87679548e25a9edf64c)
    │   └─ ← [Stop]
    ├─ [721] MockCrossChainProofOfHumanity::humanityData(0x0000000000000000000000000000000000000001000000000000000000000000) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000001, 31536001 [3.153e7], 1, true
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000001, "Humanity owner should still be owner1") [staticcall]
    │   └─ ← [Return]
    ├─ [472] MockCrossChainProofOfHumanity::receivedTransferHashes(0xf54e1cc4bb11dbb94484eb2de22518a87e125aa4a0ddf87679548e25a9edf64c) [staticcall]
    │   └─ ← [Return] true
    ├─ [0] VM::assertTrue(true, "Transfer hash should be marked as received") [staticcall]
    │   └─ ← [Return]
    ├─ [615] MockCrossChainProofOfHumanity::accountHumanity(0x0000000000000000000000000000000000000001) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000000
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, "owner1's accountHumanity should be cleared") [staticcall]
    │   └─ ← [Return]
    ├─ [2615] MockCrossChainProofOfHumanity::accountHumanity(0x0000000000000000000000000000000000000002) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000000
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, "owner2's accountHumanity should remain empty") [staticcall]
    │   └─ ← [Return]
    └─ ← [Stop]

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.14ms (693.11µs CPU time)

Revised Code Suggestion

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

import "../mocks/MockProofOfHumanity.sol";

contract MockCrossChainProofOfHumanity {
    // ... (other code remains the same)

    function receiveTransfer(address _owner, bytes20 _humanityId, uint40 _expirationTime, bytes32 _transferHash)
        external
    {
        require(!receivedTransferHashes[_transferHash], "Transfer already received");

        bool success = proofOfHumanity.ccGrantHumanity(_humanityId, _owner, _expirationTime);

        CrossChainHumanity storage humanity = humanityData[_humanityId];

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

        if (success) {
+           // Only clear the previous owner's accountHumanity if the transfer succeeds
+           if (humanity.owner != address(0)) {
+               delete accountHumanity[humanity.owner];
+           }
            accountHumanity[_owner] = _humanityId;
            humanity.owner = _owner;
            humanity.expirationTime = _expirationTime;
            humanity.isHomeChain = true;
            humanity.lastTransferTime = uint40(block.timestamp);
+           receivedTransferHashes[_transferHash] = true;
+       } else {
+           // If the transfer fails, revert the transaction
+           revert("Transfer failed: humanity already claimed");
        }

-       receivedTransferHashes[_transferHash] = true;

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

    // ... (rest of the code remains the same)
}

This fix addresses the vulnerability by:

1. Only clearing the previous owner's accountHumanity if the transfer succeeds.
2. Only marking the transfer hash as received if the transfer succeeds.
3. Reverting the transaction if the transfer fails, preventing any state changes in case of failure.

These changes ensure that the contract maintains a consistent state even when a transfer fails, mitigating the potential for exploitation or unexpected behavior.

[clesaege]

1. accountHumanity is to keep track of humanity which are bridged. In the case of a humanity being transferred, we do overwrite the info about the previous transfer. In your specific case, there is already a registered humanity, so this is the humanity which should take precedence.
2. & 3. Doing so would allow to try again in the future, so we would have a humanity "in limbo", not really valid or invalid and it could pop up in the future is the current owner were to become unregistered. This looks like a nightmare of edge cases it's better to avoid. Either it works or it doesn't.