Skip to content

fix: [M03] HubPool should send ETH to caller of removeLiquidity if they specify so #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 17, 2022
Merged

fix: [M03] HubPool should send ETH to caller of removeLiquidity if they specify so #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0a9509
fix: HubPool should send ETH to caller of removeLiquidity if they spe…
nicholaspai Mar 14, 2022
a1d1269
Update contracts/HubPool.sol
nicholaspai Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions contracts/HubPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
* @param l1Token Token to redeem LP share for.
* @param lpTokenAmount Amount of LP tokens to burn. Exchange rate between L1 token and LP token can be queried
* via public exchangeRateCurrent method.
* @param sendEth Set to True if L1 token is WETH and user wants to receive ETH.
* @param sendEth Set to True if L1 token is WETH and user wants to receive ETH. Note that if caller
* is a contract, then the contract should have a way to receive ETH if this value is set to True. Similarly,
* if this value is set to False, then the calling contract should have a way to handle WETH.
*/
function removeLiquidity(
address l1Token,
Expand All @@ -482,9 +484,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
// If they try access more funds that available (i.e l1TokensToReturn > liquidReserves) this will underflow.
pooledTokens[l1Token].liquidReserves -= l1TokensToReturn;

if (sendEth) _unwrapWETHTo(payable(msg.sender), l1TokensToReturn);
else IERC20(l1Token).safeTransfer(msg.sender, l1TokensToReturn);

if (sendEth) {
weth.withdraw(l1TokensToReturn);
payable(msg.sender).transfer(l1TokensToReturn); // This will revert if the caller is a contract that does not implement a fallback function.
} else {
IERC20(address(l1Token)).safeTransfer(msg.sender, l1TokensToReturn);
}
emit LiquidityRemoved(l1Token, l1TokensToReturn, lpTokenAmount, msg.sender);
}

Expand Down Expand Up @@ -826,16 +831,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
emit RootBundleCanceled(msg.sender, getCurrentTime(), ancillaryData);
}

// Unwraps ETH and does a transfer to a recipient address. If the recipient is a smart contract then sends WETH.
function _unwrapWETHTo(address payable to, uint256 amount) internal {
if (address(to).isContract()) {
IERC20(address(weth)).safeTransfer(to, amount);
} else {
weth.withdraw(amount);
to.transfer(amount);
}
}

function _getOptimisticOracle() internal view returns (SkinnyOptimisticOracleInterface) {
return
SkinnyOptimisticOracleInterface(finder.getImplementationAddress(OracleInterfaces.SkinnyOptimisticOracle));
Expand Down
8 changes: 4 additions & 4 deletions test/HubPool.LiquidityProvision.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ describe("HubPool Liquidity Provision", function () {
// Removing more than the total balance of LP tokens should throw.
await expect(hubPool.connect(other).removeLiquidity(dai.address, amountToLp, false)).to.be.reverted;

// Cant try receive ETH if the token is pool token is not WETH. Try redeem 1/3 of the original amount added. This is
// less than the total amount the wallet has left (since we removed half the amount before).
await expect(hubPool.connect(other).removeLiquidity(dai.address, amountToLp.div(3), true)).to.be.reverted;
// Cant try receive ETH if the token is pool token is not WETH.
await expect(hubPool.connect(other).removeLiquidity(dai.address, amountToLp.div(2), true)).to.be.reverted;

// Can remove the remaining LP tokens for a balance of 0.
// Can remove the remaining LP tokens for a balance of 0. Use the same params as the above reverted call to show
// that only the sendEth param needs to be changed.
await hubPool.connect(liquidityProvider).removeLiquidity(dai.address, amountToLp.div(2), false);
expect(await dai.balanceOf(liquidityProvider.address)).to.equal(amountToSeedWallets); // back to starting balance.
expect(await daiLpToken.balanceOf(liquidityProvider.address)).to.equal(0); // All LP tokens burnt.
Expand Down