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

Proof of Humanity Protocol v2
2 stars 1 forks source link

`send()` is not recommended for sending ether #97

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

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

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

Description: Description\

In SafeSend.sol, `

    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 issue here is that, send() function forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.

The use of the send() function for an address will inevitably make the transaction fail when:

1) The claimer smart contract does not implement a payable function. 2) The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. 3) The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300. 4) Additionally, using higher than 2300 gas might be mandatory for some multisig wallets

If we this reference too, then send() is not recommended for sending ethers

The following functions have used send() function as wrapped in safeSend():

In withdrawFeesAndRewards() and _contribute() function of ProofOfHumanity.sol

In withdrawFeesAndRewards() and _contribute() function of ProofOfHumanityExtended.sol so any functions trying sending ethers to recipient addresses would fail as mentioned in above four scenarios.

Recommendations\ Use call() instead of send() function in safeSend() function as this is the recommended way.

Note: The protocol team has confirmed any issues affecting POH from libraries would be counted.

clesaege commented 2 months ago

The whole point of SafeSend library is to wrap and send send as wrapped ERC20 the native asset if send fails. The library is exactly mitigating the concern you have in this library?