call instead of send in ProofOfHumanityOld::contribute

[hats-bug-reporter[bot]]

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

Description: Description\ fee contribution is executed with send, instead of call.

Attack Scenario\ If the msg.sender is a smart contract of Gnosis wallet, it will be impossible to make fee contribution, since only 2300 fixed gas is forwarded:

function contribute(
        Round storage _round,
        Party _side,
        address payable _contributor,
        uint256 _amount,
        uint256 _totalRequired
    ) internal returns (uint256) {
        uint256 contribution;
        uint256 remainingETH;
        (contribution, remainingETH) = calculateContribution(
            _amount,
            _totalRequired.subCap(_round.paidFees[uint256(_side)])
        );
        _round.contributions[_contributor][uint256(_side)] += contribution;
        _round.paidFees[uint256(_side)] += contribution;
        _round.feeRewards += contribution;

        if (remainingETH != 0) _contributor.send(remainingETH);//@audit

        return contribution;
    }

Attachments

1. Proof of Concept (PoC) File
2. Revised Code File (Optional)

It’s recommended to always use call for low-level calls, otherwise DoS on user level can happen.

function contribute(
        Round storage _round,
        Party _side,
        address payable _contributor,
        uint256 _amount,
        uint256 _totalRequired
    ) internal returns (uint256) {
        uint256 contribution;
        uint256 remainingETH;
        (contribution, remainingETH) = calculateContribution(
            _amount,
            _totalRequired.subCap(_round.paidFees[uint256(_side)])
        );
        _round.contributions[_contributor][uint256(_side)] += contribution;
        _round.paidFees[uint256(_side)] += contribution;
        _round.feeRewards += contribution;

-        if (remainingETH != 0) _contributor.send(remainingETH);
+        if (remainingETH != 0) {
+               (bool success, ) =  _contributor.call{value:remainingETH}("");
+                   require(success, "low level call failed)"
+                }
        return contribution;
    }

[clesaege]

Only the smart contracts of the V2 are in scope.

This looks like your are looking at a V1 contract. The function in V2 is as follows.

|-- proof-of-humanity-v2-contracts/contracts/ |-- README.md |-- ProofOfHumanity.sol |-- CrossChainProofOfHumanity.sol |-- bridge-gateways |-- AMBBridgeGateway.sol |-- extending-old |-- ForkModule.sol |-- ProofOfHumanityExtended.sol

[blckhv]

Yeah, we were looking at the wrong commit, apologies @clesaege. But in reality, you're still using the send instead of call here and issue still persist in SafeSend library:

library SafeSend {
    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);
    }
}

send will run out of gas and revert, instead of returning bool, thus making it impossible to force send _wethLike to the recipient. reference: https://solidity-by-example.org/sending-ether/

[clesaege]

The function send will not revert but will return false. You can try yourself in this example and see the doc.