Skip to content

Conversation

@nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Mar 14, 2022

Issue:

  • [M03] Confusing removeLiquidity behavior could lock funds
  • The removeLiquidity function in the HubPool contract accepts a boolean argument sendEth. This
    should be set to true "if L1 token is WETH and user wants to receive ETH".
    However, if the "user" is a smart contract, even if the L1 token is WETH and the sendEth argument is
    true, WETH, not ETH, will ultimately be sent back.
  • This is the case because if sendEth is true, then the _unwrapWETHTo function is called. That function
    checks if the intended recipient is a smart contract, and, if so, sends WETH.
    If the receiving smart contract has no mechanism to handle WETH and was only expecting ETH in
    return, as was explicitly specified by the sendEth argument submitted, then any WETH sent to such a
    contract could become inaccessible.
  • To avoid unnecessary confusion and the potential loss of funds, consider either reverting if a smart
    contract calls removeLiquidity with the sendEth argument set to true or modifying the
    _unwrapWETHTo function so that it can also be provided with and abide by an explicit sendEth
    argument.

Resolution:

  • Currently contract sends WETH to contracts even if they specify sendEth = True which is confusing and can lead to lost funds by contracts who cannot handle WETH and don't read the contract source code closely. This PR sends ETH or WETH to caller as specified

@nicholaspai nicholaspai added the OZ Audit - March Resolves issue discovered in March 2022 OZ Audit label Mar 15, 2022
@nicholaspai nicholaspai changed the title fix: HubPool should send ETH to caller of removeLiquidity if they specify so fix: [M03] HubPool should send ETH to caller of removeLiquidity if they specify so Mar 15, 2022
Copy link
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good solution. we are saved by virtue of smart contracts that dont implement fallback functions reverting if you try send them eth. The way you have it now is most locally consistant and will enable both kinds of flow.

LGTM

Co-authored-by: Chris Maree <christopher.maree@gmail.com>
@nicholaspai nicholaspai requested a review from mrice32 March 16, 2022 15:25
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OZ Audit - March Resolves issue discovered in March 2022 OZ Audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants