Humanity can be lost when transferred to a foreign chain

[hats-bug-reporter[bot]]

Github username: @Audinarey Twitter username: audinarey Submission hash (on-chain): 0x70dd7f41ed08977905011ceca91801ba7bedd3c6dfb70df3eefda6f75e298992 Severity: high

Description: Description\ Users can call crossChainProofOfHumanity::transferHumanity(...) to transfer humanity from the current chain in which it exist to another chain. However, due to the implementation of the transferHumanity(...) function, the humanity could be lost

Attack Scenario\ As shown below on L281, after the necessary validation has been done on the humanity to send, the BridgeGateway is meant to call the ICrossChainProofOfHumanity.receiveTransfer() finvtion on the destination chain with the new owner set to msg.sender which is the owner on the origin/source chain.

File: CrossChainProofOfHumanity.sol
250:     function transferHumanity(address _bridgeGateway) external allowedGateway(_bridgeGateway) {
251:         // Function will require humanity to be claimed by sender, have no pending requests and human not vouching for others at the time
252:         (bytes20 humanityId, uint40 expirationTime) = proofOfHumanity.ccDischargeHumanity(msg.sender);

SNIP .....

277: 
278:         IBridgeGateway(_bridgeGateway).sendMessage( // @audit does not check if the bridge gateway is paused or something
279:             abi.encodeWithSelector(
280:                 ICrossChainProofOfHumanity.receiveTransfer.selector,
281:   @>         msg.sender,
282:                 humanityId,
283:                 expirationTime,
284:                 tHash
285:             )
286:         );
287: 
288:         emit TransferInitiated(humanityId, msg.sender, expirationTime, _bridgeGateway, tHash);
289:     }

The problem is that msg.sender which is the owner of the humanity on the current chain may be controlled by a different user on a different chain and as such the ownership of the transfered humanity will be lost to a new owner if the humanity has not been previously claimed in the new chain or the new owner has not been registered in the forked module.

File: ProofOfHumanityExtended.sol
503:     function ccGrantHumanity(
504:         bytes20 _humanityId,
505:         address _account,
506:         uint40 _expirationTime
507:     ) external onlyCrossChain returns (bool success) {
....
509: 
510:         // If humanity is claimed, don't overwrite.
511:         if (
512:             (humanity.owner != address(0x0) && block.timestamp < humanity.expirationTime) ||
513:             // If not claimed in this contract, check in fork module too.
514:             forkModule.isRegistered(_account)
515:         ) return false;
.......
519: 
520:    @>   humanity.owner = _account;
.......
527:     }

File: CrossChainProofOfHumanity.sol
332:     function receiveTransfer(
333:         address _owner,
334:         bytes20 _humanityId,
335:         uint40 _expirationTime,
336:         bytes32 _transferHash
337:     ) external override allowedGateway(msg.sender) {
.........
350:         // Overriding this data in case it is outdated
351:         if (success) {
352:             accountHumanity[_owner] = _humanityId;
353: 
354:     @>      humanity.owner = _owner;
......
358:         }
.....
363:     }

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Modify the crossChainProofOfHumanity::transferHumanity(...) function as shown below

File: CrossChainProofOfHumanity.sol
-250:     function transferHumanity(address _bridgeGateway) external allowedGateway(_bridgeGateway) {
+250:     function transferHumanity(address _bridgeGateway, address recipient) external allowedGateway(_bridgeGateway) {
251:         // Function will require humanity to be claimed by sender, have no pending requests and human not vouching for others at the time
252:         (bytes20 humanityId, uint40 expirationTime) = proofOfHumanity.ccDischargeHumanity(msg.sender);

SNIP .....

277: 
278:         IBridgeGateway(_bridgeGateway).sendMessage( // @audit does not check if the bridge gateway is paused or something
279:             abi.encodeWithSelector(
280:                 ICrossChainProofOfHumanity.receiveTransfer.selector,
-281:               msg.sender,
+281:              recipient,
282:                 humanityId,
283:                 expirationTime,
284:                 tHash
285:             )
286:         );
287: 
288:         emit TransferInitiated(humanityId, msg.sender, expirationTime, _bridgeGateway, tHash);
289:     }

[Audinarey]

Kindly ignore this finding as I had to resubmit it in #56 because I could not edit it.