code423n4 commented 2 years ago

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 commented 2 years ago

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

mehtaculous commented 2 years ago

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 commented 2 years ago

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.