Off-by-One Error in Humanity Expiration Time Checks

[hats-bug-reporter[bot]]

Github username: -- Twitter username: dod4ufn Submission hash (on-chain): 0x28e7489aaa6f851dab0f1ef245b5887370a9d7572b54056fe109248e2e373a35 Severity: low

Description: Description\ There is an inconsistency in how the contract checks the expiration time of a humanity across different functions. Specifically, the isClaimed and boundTo functions use >= to compare the expirationTime with the current timestamp, while the isHuman function uses a > comparison. This discrepancy can result in an off-by-one error where a humanity that has an expiration time exactly equal to the current timestamp. Such inconsistency can lead to unexpected behavior and potential vulnerabilities in the contract's logic.

1. Proof of Concept (PoC)

In the CrossChainProofOfHumanity.sol file:

function isClaimed(bytes20 _humanityId) external view returns (bool) {
    if (proofOfHumanity.isClaimed(_humanityId)) return true;

    CrossChainHumanity memory humanity = humanityData[_humanityId];
@>  return humanity.owner != address(0) && humanity.expirationTime >= block.timestamp;
}

function isHuman(address _account) external view returns (bool) {
    if (proofOfHumanity.isHuman(_account)) return true;

    bytes20 humanityId = accountHumanity[_account];
    CrossChainHumanity memory humanity = humanityData[humanityId];

    return !humanity.isHomeChain
        && humanityId != bytes20(0x0)
        && humanity.owner == _account
@>      && humanity.expirationTime > block.timestamp;
}

function boundTo(bytes20 _humanityId) external view returns (address owner) {
    owner = proofOfHumanity.boundTo(_humanityId);

    if (owner == address(0x0)) {
        CrossChainHumanity memory humanity = humanityData[_humanityId];

@>      if (humanity.expirationTime >= block.timestamp) owner = humanity.owner;
    }
}

2. Revised Code

Implement a "centralized" function that checks the validity of data and is used in all the other relevant functions that use these checks and update the data.

[clesaege]

This looks correct and I could see a contract calling isClaimed and isHuman in the same transaction leading to unexpected behaviour.

I'll double check, but this does look valid.