When the user is a smart contract, the user may not be able to withdraw ETH through the withdraw function

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L44-L50

Vulnerability details

Impact

In the Exchange contract, when paymentToken == POOL, the user's balance in the Pool contract will be increased.
In the Pool contract, the wrapped ETH cannot be transferred by the owner and can be withdrawn only by the owner himself.
If the user is a smart contract and will be reverted in the fallback/receive function, the user's ETH will not be able to be withdrawn.
This vulnerability is similar to the one I previously reported in foundation, which is marked as medium.
code-423n4/2022-02-foundation-findings#12

Proof of Concept

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L44-L51

Tools Used

None

Recommended Mitigation Steps

Consider allowing the user to specify the recipient in the withdraw function

[berndartmueller]

The paymentToken is known and agreed upon by both involved parties (buyer and seller). If the receiver of the POOL tokens is a smart contract unable to withdraw ETH from the pool, it is the fault of the involved party. Thus I'm inclined to downgrade to QA (Low-risk).

But for now, I'm leaving it as is for the sponsor review. The final judgment follows after receiving feedback from the sponsor.

[nonfungible47]

Agree with above, users will need to ensure they can receive their funds properly.

[c4-sponsor]

nonfungible47 marked the issue as disagree with severity

[berndartmueller]

Downgrading to QA (Low) as both involved parties are aware of the paymentToken and the involved risks.

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c