hats-finance / Proof-Of-Humanity-V2-0xef0709445d394a22704850c772a28a863bb780b0

Proof of Humanity Protocol v2
2 stars 1 forks source link

Withdrawal fee and rewards will be stuck in the contract #61

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

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

Description: Description\ Users call ProofOfHumanity::withdrawFeesAndRewards(...) if no disputes were raised to reimburse contributions.

Attack Scenario\

However, as shown below, the ProofOfHumanity::withdrawFeesAndRewards(...) function sends the reimbursement by calling safeSend(...)

File: ProofOfHumanity.sol
1251:     function withdrawFeesAndRewards(
1252:         address payable _beneficiary,
1253:         bytes20 _humanityId,
1254:         uint256 _requestId,
1255:         uint256 _challengeId,
1256:         uint256 _round
1257:     ) public {
.....
1295: @>   _beneficiary.safeSend(reward, wNative);
1296: 
1297:         emit FeesAndRewardsWithdrawn(_humanityId, _requestId, _challengeId, _round, _beneficiary);
1298:     }

Safesend uses transfer(...)which send exactly 2300 gas with the transaction.

function safeSend(address payable _to, uint256 _value, address _wethLike) internal {
        if (_to.send(_value)) return;

        WethLike(_wethLike).deposit{value: _value}();
  @>    WethLike(_wethLike).transfer(_to, _value);
    }

The problem is that if the _beneficiary is a contract account like a multisig or smart contract wallet (e.g this safe can use up to 6000 gas) the call will revert as the 2300 gas is not enough for the call to proceed. The will cause the funds to be stuck in the POH contract.

Revised Code File (Optional)

Modify the safeSend(...) function to use call(...) instead of transfer

clesaege commented 2 months ago

If the send fails, the native asset will be wrapped and sent as ERC20. This is the whole point of the SafeSend library.