Loss of ETH for proposer when it is a contract that doesn't have fallback function.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L209

Vulnerability details

Description

sendEthOrWeth() is used in several locations in OptimisticListingSeaport:

1. rejectProposal - sent to proposer
2. rejectActive - sent to proposer
3. cash - sent to msg.sender

This is the implementation of sendEthOrWeth:

function _attemptETHTransfer(address _to, uint256 _value) internal returns (bool success) {
    // Here increase the gas limit a reasonable amount above the default, and try
    // to send ETH to the recipient.
    // NOTE: This might allow the recipient to attempt a limited reentrancy attack.
    (success, ) = _to.call{value: _value, gas: 30000}("");
}
/// @notice Sends eth or weth to an address
/// @param _to Address to send to
/// @param _value Amount to send
function _sendEthOrWeth(address _to, uint256 _value) internal {
    if (!_attemptETHTransfer(_to, _value)) {
        WETH(WETH_ADDRESS).deposit{value: _value}();
        WETH(WETH_ADDRESS).transfer(_to, _value);
    }
}

The issue is that the receive could be a contract that does not have a fallback function. In this scenario, _attemptETHTransfer will fail and WETH would be transferred to the contract. It is likely that it bricks those funds for the contract as there is no reason it would support interaction with WETH tokens.

It can be reasonably assumed that developers will develop contracts which will interact with OptimisticListingSeaport using proposals. They are not warned and are likely to suffer losses.

Impact

Loss of ETH for proposer when it is a contract that doesn't have fallback function.

Tools Used

Manual audit

Recommended Mitigation Steps

Either enforce that proposer is an EOA or take in a recipient address for ETH transfers.

[HickupHH3]

QA issue IMO. Seems reasonable to expect the proposer / RAE holder participating to be able to handle ETH in the first place.

Once the listing is purchased, the vault will act similarly to an Optimistic Buyout. The proposer will have their Raes returned to them, and all users can burn their Raes for their portion of ETH gained from the sale.

Will await sponsor input before making a final decision.

[c4-judge]

HickupHH3 marked the issue as primary issue

[c4-judge]

HickupHH3 marked the issue as satisfactory

[trust1995]

QA issue IMO. Seems reasonable to expect the proposer / RAE holder participating to be able to handle ETH in the first place.

Once the listing is purchased, the vault will act similarly to an Optimistic Buyout. The proposer will have their Raes returned to them, and all users can burn their Raes for their portion of ETH gained from the sale.

Will await sponsor input before making a final decision.

It is not about ETH handling, because if it doesn't have a receive() method it would not receive the ETH in the first place in _attemptETHTransfer. It would get WETH credit instead but we can't expect contracts to support arbitrary WETH<->ETH conversion. I think it is a loss of funds with stated hypotheticals which doesn't require a user mistake.

Update: It is a similar issue to #6 , both assume contract fallback is implemented.

[HickupHH3]

I'm confused, the title and description's premise is that the contract doesn't have a fallback / receive function right? Not sure what you mean by "both assume contract fallback is implemented". My stance is that this premise is questionable because the README clearly stated that ETH will be claimed in exchange for RAEs.

6 is different; it's arguing that the tx won't revert even if the logic inside fallback / receive function of the receiver contract does, which would be catastrophic.

[trust1995]

Not sure what you mean by "both assume contract fallback is implemented"

In this report, if sender contract doesn't implement fallback it will be loss of funds. Same thing happens in issue #6 if sender doesn't implement it. I accept the argument that this doesn't seem likely but loss of funds in stated hypotheticals can be accepted as M-level.

it's arguing that the tx won't revert even if the logic inside fallback / receive function of the receiver contract does, which would be catastrophic.

Described scenario also applies to this issue. It is not detailed in the submission but it's still a major concern like in #6. If sponsor would address _sendEthOrWeth issue it would fix both the hypothetical and the little-less-hypothetical attack.

[HickupHH3]

The main problem is using WETH as a fallback for failed ETH transfers, which I agree with. However, I disagree with the premise being a failure to accept ETH. I would've had no qualms with the issue if the premise was a revert in the fallback function instead.

I'd like the sponsor to give their input before commenting further.

[stevennevins]

This and #6 seem like distinct issues to me

[c4-sponsor]

stevennevins marked the issue as disagree with severity

[c4-sponsor]

stevennevins marked the issue as sponsor confirmed

[HickupHH3]

Yes, distinct issues. The argument here is about the contract being able to handle ETH but not WETH. If the ETH transfer fails (eg. gas used exceeds the 30k sent), then funds would be stuck.

On the fence regarding severity here.

[stevennevins]

I actually more agree with this being an issue:

Yes, distinct issues. The argument here is about the contract being able to handle ETH but not WETH. If the ETH transfer fails (eg. gas used exceeds the 30k sent), then funds would be stuck.

But it's not clear to me that is what was originally highlighted in the description of the issue

[HickupHH3]

Yeah it's not fully clear because the premise is the contract not having a fallback function, but the intended effect of not being able to handle WETH is.

It is likely that it bricks those funds for the contract as there is no reason it would support interaction with WETH tokens.

[c4-judge]

HickupHH3 marked the issue as selected for report