From a54f9109288d0c4398b87c564b23250b57caf119 Mon Sep 17 00:00:00 2001 From: David Racero Date: Tue, 3 Nov 2020 17:23:35 +0100 Subject: [PATCH] Added emergency token and ether withdrawal. Follow code guidelines. --- contracts/misc/WETHGateway.sol | 62 ++++++++-- .../mocks/attacks/SefldestructTransfer.sol | 8 ++ deployed-contracts.json | 17 ++- helpers/contracts-deployments.ts | 10 ++ helpers/contracts-getters.ts | 9 ++ helpers/types.ts | 1 + test/weth-gateway.spec.ts | 112 ++++++++++++++++-- 7 files changed, 199 insertions(+), 20 deletions(-) create mode 100644 contracts/mocks/attacks/SefldestructTransfer.sol diff --git a/contracts/misc/WETHGateway.sol b/contracts/misc/WETHGateway.sol index 1ffd290a..61792110 100644 --- a/contracts/misc/WETHGateway.sol +++ b/contracts/misc/WETHGateway.sol @@ -10,14 +10,16 @@ import {ReserveLogic} from '../libraries/logic/ReserveLogic.sol'; import {ReserveConfiguration} from '../libraries/configuration/ReserveConfiguration.sol'; import {UserConfiguration} from '../libraries/configuration/UserConfiguration.sol'; import {Helpers} from '../libraries/helpers/Helpers.sol'; +import {Ownable} from '../dependencies/openzeppelin/contracts/Ownable.sol'; +import {IERC20} from '../dependencies/openzeppelin/contracts/IERC20.sol'; -contract WETHGateway is IWETHGateway { +contract WETHGateway is IWETHGateway, Ownable { using ReserveConfiguration for ReserveConfiguration.Map; using UserConfiguration for UserConfiguration.Map; - IWETH public immutable WETH; - ILendingPool public immutable POOL; - IAToken public immutable aWETH; + IWETH internal immutable WETH; + ILendingPool internal immutable POOL; + IAToken internal immutable aWETH; /** * @dev Sets the WETH address and the LendingPoolAddressesProvider address. Infinite approves lending pool. @@ -59,7 +61,7 @@ contract WETHGateway is IWETHGateway { aWETH.transferFrom(msg.sender, address(this), amountToWithdraw); POOL.withdraw(address(WETH), amountToWithdraw, address(this)); WETH.withdraw(amountToWithdraw); - safeTransferETH(to, amountToWithdraw); + _safeTransferETH(to, amountToWithdraw); } /** @@ -91,7 +93,7 @@ contract WETHGateway is IWETHGateway { POOL.repay(address(WETH), msg.value, rateMode, onBehalfOf); // refund remaining dust eth - if (msg.value > paybackAmount) safeTransferETH(msg.sender, msg.value - paybackAmount); + if (msg.value > paybackAmount) _safeTransferETH(msg.sender, msg.value - paybackAmount); } /** @@ -99,11 +101,57 @@ contract WETHGateway is IWETHGateway { * @param to recipient of the transfer * @param value the amount to send */ - function safeTransferETH(address to, uint256 value) internal { + function _safeTransferETH(address to, uint256 value) internal { (bool success, ) = to.call{value: value}(new bytes(0)); require(success, 'ETH_TRANSFER_FAILED'); } + /** + * @dev transfer ERC20 from the utility contract, for ERC20 recovery in case of stuck tokens due + * direct transfers to the contract address. + * @param token token to transfer + * @param to recipient of the transfer + * @param amount amount to send + */ + function emergencyTokenTransfer( + address token, + address to, + uint256 amount + ) external onlyOwner { + IERC20(token).transfer(to, amount); + } + + /** + * @dev transfer native Ether from the utility contract, for native Ether recovery in case of stuck Ether + * due selfdestructs, transfer ether to pre-computated contract address before deployment or bad contract behaviour. + * @param to recipient of the transfer + * @param amount amount to send + */ + function emergencyEtherTransfer(address to, uint256 amount) external onlyOwner { + _safeTransferETH(to, amount); + } + + /** + * @dev Get WETH address used by WETHGateway + */ + function getWETHAddress() external view returns (address) { + return address(WETH); + } + + /** + * @dev Get aWETH address used by WETHGateway + */ + function getAWETHAddress() external view returns (address) { + return address(aWETH); + } + + /** + * @dev Get LendingPool address used by WETHGateway + */ + function getLendingPoolAddress() external view returns (address) { + return address(POOL); + } + /** * @dev Only WETH contract is allowed to transfer ETH here. Prevent other addresses to send Ether to this contract. */ diff --git a/contracts/mocks/attacks/SefldestructTransfer.sol b/contracts/mocks/attacks/SefldestructTransfer.sol new file mode 100644 index 00000000..5c8750c1 --- /dev/null +++ b/contracts/mocks/attacks/SefldestructTransfer.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: agpl-3.0 +pragma solidity ^0.6.8; + +contract SelfdestructTransfer { + function destroyAndTransfer(address payable to) external payable { + selfdestruct(to); + } +} diff --git a/deployed-contracts.json b/deployed-contracts.json index 431c5322..1ebb0d2d 100644 --- a/deployed-contracts.json +++ b/deployed-contracts.json @@ -171,7 +171,7 @@ }, "ReserveLogic": { "buidlerevm": { - "address": "0x920d847fE49E54C19047ba8bc236C45A8068Bca7", + "address": "0xFAe0fd738dAbc8a0426F47437322b6d026A9FD95", "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" }, "kovan": { @@ -181,20 +181,19 @@ }, "GenericLogic": { "buidlerevm": { - "address": "0xA4765Ff72A9F3CfE73089bb2c3a41B838DF71574", + "address": "0x6082731fdAba4761277Fb31299ebC782AD3bCf24", "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" } }, "ValidationLogic": { "buidlerevm": { - "address": "0x35c1419Da7cf0Ff885B8Ef8EA9242FEF6800c99b", + "address": "0x8456161947DFc1fC159A0B26c025cD2b4bba0c3e", "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" } }, "LendingPool": { "buidlerevm": { - "address": "0xe2607EabC87fd0A4856840bF23da8458cDF0434F", - "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" + "address": "0xD9273d497eDBC967F39d419461CfcF382a0A822e" } }, "LendingPoolConfigurator": { @@ -210,7 +209,7 @@ }, "ATokensAndRatesHelper": { "buidlerevm": { - "address": "0xA4765Ff72A9F3CfE73089bb2c3a41B838DF71574", + "address": "0x06bA8d8af0dF898D0712DffFb0f862cC51AF45c2", "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" } }, @@ -301,5 +300,11 @@ "address": "0x920d847fE49E54C19047ba8bc236C45A8068Bca7", "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" } + }, + "SelfdestructTransferMock": { + "buidlerevm": { + "address": "0x920d847fE49E54C19047ba8bc236C45A8068Bca7", + "deployer": "0xc783df8a850f42e7F7e57013759C285caa701eB6" + } } } diff --git a/helpers/contracts-deployments.ts b/helpers/contracts-deployments.ts index bbda1e2d..610104df 100644 --- a/helpers/contracts-deployments.ts +++ b/helpers/contracts-deployments.ts @@ -41,6 +41,7 @@ import { MockVariableDebtTokenFactory, PriceOracleFactory, ReserveLogicFactory, + SelfdestructTransferFactory, StableDebtTokenFactory, VariableDebtTokenFactory, WalletBalanceProviderFactory, @@ -53,6 +54,7 @@ import {StableAndVariableTokensHelperFactory} from '../types/StableAndVariableTo import {MockStableDebtToken} from '../types/MockStableDebtToken'; import {MockVariableDebtToken} from '../types/MockVariableDebtToken'; import {MintableDelegationErc20} from '../types/MintableDelegationErc20'; +import {SelfdestructTransfer} from '../types/SelfdestructTransfer'; export const deployLendingPoolAddressesProvider = async (verify?: boolean) => withSaveAndVerify( @@ -472,3 +474,11 @@ export const deployMockAToken = async ( args, verify ); + +export const deploySelfdestructTransferMock = async (verify?: boolean) => + withSaveAndVerify( + await new SelfdestructTransferFactory(await getFirstSigner()).deploy(), + eContractid.SelfdestructTransferMock, + [], + verify + ); diff --git a/helpers/contracts-getters.ts b/helpers/contracts-getters.ts index 3e7ddf9a..ee70aece 100644 --- a/helpers/contracts-getters.ts +++ b/helpers/contracts-getters.ts @@ -16,6 +16,7 @@ import { MockVariableDebtTokenFactory, PriceOracleFactory, ReserveLogicFactory, + SelfdestructTransferFactory, StableAndVariableTokensHelperFactory, StableDebtTokenFactory, VariableDebtTokenFactory, @@ -262,3 +263,11 @@ export const getMockStableDebtToken = async (address?: tEthereumAddress) => (await getDb().get(`${eContractid.MockStableDebtToken}.${BRE.network.name}`).value()).address, await getFirstSigner() ); + +export const getSelfdestructTransferMock = async (address?: tEthereumAddress) => + await SelfdestructTransferFactory.connect( + address || + (await getDb().get(`${eContractid.SelfdestructTransferMock}.${BRE.network.name}`).value()) + .address, + await getFirstSigner() + ); diff --git a/helpers/types.ts b/helpers/types.ts index c9158fd6..c4b57c00 100644 --- a/helpers/types.ts +++ b/helpers/types.ts @@ -62,6 +62,7 @@ export enum eContractid { WETHGateway = 'WETHGateway', WETH = 'WETH', WETHMocked = 'WETHMocked', + SelfdestructTransferMock = 'SelfdestructTransferMock', } /* diff --git a/test/weth-gateway.spec.ts b/test/weth-gateway.spec.ts index 5e478600..cf23a042 100644 --- a/test/weth-gateway.spec.ts +++ b/test/weth-gateway.spec.ts @@ -1,11 +1,13 @@ import {MAX_UINT_AMOUNT} from '../helpers/constants'; import {convertToCurrencyDecimals} from '../helpers/contracts-helpers'; import {makeSuite, TestEnv} from './helpers/make-suite'; -import {parseEther} from 'ethers/lib/utils'; +import {formatEther, parseEther, parseUnits} from 'ethers/lib/utils'; import {BRE, waitForTx} from '../helpers/misc-utils'; import {BigNumber} from 'ethers'; import {getStableDebtToken, getVariableDebtToken} from '../helpers/contracts-getters'; import {WethGateway} from '../types/WethGateway'; +import {use} from 'chai'; +import {deploySelfdestructTransferMock} from '../helpers/contracts-deployments'; const {expect} = require('chai'); @@ -183,27 +185,123 @@ makeSuite('Use native ETH at LendingPool via WETHGateway', (testEnv: TestEnv) => expect(debtBalanceAfterFullRepay).to.be.eq(zero); }); - it('Should revert if receive or fallback functions receives Ether', async () => { + it('Should revert if receiver function receives Ether if not WETH', async () => { const {users, wethGateway} = testEnv; const user = users[0]; const amount = parseEther('1'); // Call receiver function (empty data + value) await expect( - user.signer.sendTransaction({to: wethGateway.address, value: amount}) + user.signer.sendTransaction({ + to: wethGateway.address, + value: amount, + gasLimit: BRE.network.config.gas, + }) ).to.be.revertedWith('Receive not allowed'); + }); + it('Should revert if fallback functions is called with Ether', async () => { + const {users, wethGateway} = testEnv; + const user = users[0]; + const amount = parseEther('1'); const fakeABI = ['function wantToCallFallback()']; const abiCoder = new BRE.ethers.utils.Interface(fakeABI); const fakeMethodEncoded = abiCoder.encodeFunctionData('wantToCallFallback', []); // Call fallback function with value await expect( - user.signer.sendTransaction({to: wethGateway.address, data: fakeMethodEncoded, value: amount}) - ).to.be.reverted; + user.signer.sendTransaction({ + to: wethGateway.address, + data: fakeMethodEncoded, + value: amount, + gasLimit: BRE.network.config.gas, + }) + ).to.be.revertedWith('Fallback not allowed'); + }); + + it('Should revert if fallback functions is called', async () => { + const {users, wethGateway} = testEnv; + const user = users[0]; + + const fakeABI = ['function wantToCallFallback()']; + const abiCoder = new BRE.ethers.utils.Interface(fakeABI); + const fakeMethodEncoded = abiCoder.encodeFunctionData('wantToCallFallback', []); // Call fallback function without value - await expect(user.signer.sendTransaction({to: wethGateway.address, data: fakeMethodEncoded})).to - .be.reverted; + await expect( + user.signer.sendTransaction({ + to: wethGateway.address, + data: fakeMethodEncoded, + gasLimit: BRE.network.config.gas, + }) + ).to.be.revertedWith('Fallback not allowed'); + }); + + it('Getters should retrieve correct state', async () => { + const {aWETH, weth, pool, wethGateway} = testEnv; + + const WETHAddress = await wethGateway.getWETHAddress(); + const aWETHAddress = await wethGateway.getAWETHAddress(); + const poolAddress = await wethGateway.getLendingPoolAddress(); + + expect(WETHAddress).to.be.equal(weth.address); + expect(aWETHAddress).to.be.equal(aWETH.address); + expect(poolAddress).to.be.equal(pool.address); + }); + + it('Owner can do emergency token recovery', async () => { + const {users, dai, wethGateway, deployer} = testEnv; + const user = users[0]; + const amount = parseEther('1'); + + await dai.connect(user.signer).mint(amount); + const daiBalanceAfterMint = await dai.balanceOf(user.address); + + await dai.connect(user.signer).transfer(wethGateway.address, amount); + const daiBalanceAfterBadTransfer = await dai.balanceOf(user.address); + expect(daiBalanceAfterBadTransfer).to.be.eq( + daiBalanceAfterMint.sub(amount), + 'User should have lost the funds here.' + ); + + await wethGateway + .connect(deployer.signer) + .emergencyTokenTransfer(dai.address, user.address, amount); + const daiBalanceAfterRecovery = await dai.balanceOf(user.address); + + expect(daiBalanceAfterRecovery).to.be.eq( + daiBalanceAfterMint, + 'User should recover the funds due emergency token transfer' + ); + }); + + xit('Owner can do emergency native ETH recovery', async () => { + const {users, wethGateway, deployer} = testEnv; + const user = users[0]; + const amount = parseEther('1'); + + const selfdestructContract = await deploySelfdestructTransferMock(); + + const userBalancePriorCall = await user.signer.getBalance(); + const callTx = await selfdestructContract.destroyAndTransfer(wethGateway.address, { + value: amount, + }); + const {gasUsed} = await waitForTx(callTx); + const gasFees = gasUsed.mul(callTx.gasPrice); + const userBalanceAfterCall = await user.signer.getBalance(); + console.log(formatEther(userBalanceAfterCall)); + + expect(userBalanceAfterCall).to.be.eq(userBalancePriorCall.sub(amount).sub(gasFees), ''); + 'User should have lost the funds'; + + await wethGateway.connect(deployer.signer).emergencyEtherTransfer(user.address, amount); + + const userBalanceAfterRecovery = await user.signer.getBalance(); + + console.log(formatEther(userBalanceAfterCall)); + expect(userBalanceAfterRecovery).to.be.eq( + userBalancePriorCall.sub(gasFees), + 'User should recover the funds due emergency eth transfer' + ); }); });