Receiving address might not be able to handle WETH instead of ETH

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/SafeSend.sol#L30-L35

Vulnerability details

Impact

Funds might be automatically frozen via _sendEthOrWeth().

Proof of Concept

buyFractions() does not provide the option to pay with WETH, therefore the receiving address of sellFractions() wouldn't expect to receive WETH instead of ETH.

If for some unexpected reason the native ETH transfer fails, WETH will be sent to the receiver that might not be able to handle ERC20s, leading to the funds being frozen.

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/SafeSend.sol#L30-L35

    function _sendEthOrWeth(address _to, uint256 _value) internal {
        if (!_attemptETHTransfer(_to, _value)) {
            WETH(WETH_ADDRESS).deposit{value: _value}();
            WETH(WETH_ADDRESS).transfer(_to, _value);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Simply revert if native ETH transfer is not successful instead of attempting WETH transfer.

Other instance:
https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L235-L236

[itsmetechjay]

Warden submitted this as a high-risk severity by mistake, so we are updating it to medium severity.

[mehtaculous]

0 (Not bug)

WETH is used in the event that the receiving address is not an EOA and it has not implemented a payable receive function to receive eth. This is not an issue.

[HardlyDifficult]

There's some validity to the warden's point here. Where it wouldn't lead to assets getting trapped in the contract, reverting could be a safer solution than falling back to WETH. Lowing risk and converting into a QA report for the warden.

[HardlyDifficult]

Merging with #605, #604