From 37a9c7ad885b95bcfe4de35fc5f33d1e43ad510e Mon Sep 17 00:00:00 2001 From: eboado Date: Wed, 9 Sep 2020 13:06:46 +0200 Subject: [PATCH] - Added reentrancy guard on repayWithCollateral() and test. --- contracts/lendingpool/LendingPool.sol | 9 ++++++- .../LendingPoolLiquidationManager.sol | 5 ++++ contracts/mocks/flashloan/MockSwapAdapter.sol | 27 +++++++++++++++---- test/repay-with-collateral.spec.ts | 26 ++++++++++++++++++ 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/contracts/lendingpool/LendingPool.sol b/contracts/lendingpool/LendingPool.sol index 1d266080..9b9372f2 100644 --- a/contracts/lendingpool/LendingPool.sol +++ b/contracts/lendingpool/LendingPool.sol @@ -51,6 +51,8 @@ contract LendingPool is ReentrancyGuard, VersionedInitializable, ILendingPool { address[] internal _reservesList; + bool internal _flashLiquidationLocked; + /** * @dev only lending pools configurator can use functions affected by this modifier **/ @@ -474,7 +476,10 @@ contract LendingPool is ReentrancyGuard, VersionedInitializable, ILendingPool { uint256 principalAmount, address receiver, bytes calldata params - ) external override nonReentrant { + ) external override { + require(!_flashLiquidationLocked, "REENTRANCY_NOT_ALLOWED"); + _flashLiquidationLocked = true; + address liquidationManager = _addressesProvider.getLendingPoolLiquidationManager(); //solium-disable-next-line @@ -496,6 +501,8 @@ contract LendingPool is ReentrancyGuard, VersionedInitializable, ILendingPool { if (returnCode != 0) { revert(string(abi.encodePacked(returnMessage))); } + + _flashLiquidationLocked = false; } /** diff --git a/contracts/lendingpool/LendingPoolLiquidationManager.sol b/contracts/lendingpool/LendingPoolLiquidationManager.sol index b79bfed7..888edb93 100644 --- a/contracts/lendingpool/LendingPoolLiquidationManager.sol +++ b/contracts/lendingpool/LendingPoolLiquidationManager.sol @@ -37,6 +37,9 @@ contract LendingPoolLiquidationManager is ReentrancyGuard, VersionedInitializabl using ReserveConfiguration for ReserveConfiguration.Map; using UserConfiguration for UserConfiguration.Map; + // IMPORTANT The storage layout of the LendingPool is reproduced here because this contract + // is gonna be used through DELEGATECALL + LendingPoolAddressesProvider internal addressesProvider; mapping(address => ReserveLogic.ReserveData) internal reserves; @@ -44,6 +47,8 @@ contract LendingPoolLiquidationManager is ReentrancyGuard, VersionedInitializabl address[] internal reservesList; + bool internal _flashLiquidationLocked; + uint256 internal constant LIQUIDATION_CLOSE_FACTOR_PERCENT = 5000; /** diff --git a/contracts/mocks/flashloan/MockSwapAdapter.sol b/contracts/mocks/flashloan/MockSwapAdapter.sol index 6b2f2330..85c7d84b 100644 --- a/contracts/mocks/flashloan/MockSwapAdapter.sol +++ b/contracts/mocks/flashloan/MockSwapAdapter.sol @@ -4,11 +4,13 @@ pragma solidity ^0.6.8; import {MintableERC20} from '../tokens/MintableERC20.sol'; import {ILendingPoolAddressesProvider} from '../../interfaces/ILendingPoolAddressesProvider.sol'; import {ISwapAdapter} from '../../interfaces/ISwapAdapter.sol'; +import {ILendingPool} from "../../interfaces/ILendingPool.sol"; import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol'; contract MockSwapAdapter is ISwapAdapter { - uint256 amountToReturn; + uint256 internal _amountToReturn; + bool internal _tryReentrancy; ILendingPoolAddressesProvider public addressesProvider; event Swapped(address fromAsset, address toAsset, uint256 fromAmount, uint256 receivedAmount); @@ -18,7 +20,11 @@ contract MockSwapAdapter is ISwapAdapter { } function setAmountToReturn(uint256 amount) public { - amountToReturn = amount; + _amountToReturn = amount; + } + + function setTryReentrancy(bool tryReentrancy) public { + _tryReentrancy = tryReentrancy; } function executeOperation( @@ -30,10 +36,21 @@ contract MockSwapAdapter is ISwapAdapter { ) external override { params; IERC20(assetToSwapFrom).transfer(address(1), amountToSwap); // We don't want to keep funds here - MintableERC20(assetToSwapTo).mint(amountToReturn); - IERC20(assetToSwapTo).approve(fundsDestination, amountToReturn); + MintableERC20(assetToSwapTo).mint(_amountToReturn); + IERC20(assetToSwapTo).approve(fundsDestination, _amountToReturn); - emit Swapped(assetToSwapFrom, assetToSwapTo, amountToSwap, amountToReturn); + if (_tryReentrancy) { + ILendingPool(fundsDestination).repayWithCollateral( + assetToSwapFrom, + assetToSwapTo, + address(1), // Doesn't matter, we just want to test the reentrancy + 1 ether, // Same + address(1), // Same + "0x" + ); + } + + emit Swapped(assetToSwapFrom, assetToSwapTo, amountToSwap, _amountToReturn); } function burnAsset(IERC20 asset, uint256 amount) public { diff --git a/test/repay-with-collateral.spec.ts b/test/repay-with-collateral.spec.ts index 0bd25680..d7f9d77f 100644 --- a/test/repay-with-collateral.spec.ts +++ b/test/repay-with-collateral.spec.ts @@ -67,12 +67,38 @@ makeSuite('LendingPool. repayWithCollateral()', (testEnv: TestEnv) => { await pool.connect(user.signer).borrow(dai.address, amountToBorrow, 2, 0); }); + it('It is not possible to do reentrancy on repayWithCollateral()', async () => { + const {pool, weth, dai, users, mockSwapAdapter, oracle} = testEnv; + const user = users[1]; + + const amountToRepay = parseEther('10'); + + await waitForTx(await mockSwapAdapter.setTryReentrancy(true)) + + await mockSwapAdapter.setAmountToReturn(amountToRepay); + await expect( + pool + .connect(user.signer) + .repayWithCollateral( + weth.address, + dai.address, + user.address, + amountToRepay, + mockSwapAdapter.address, + '0x' + ) + ).to.be.revertedWith("FAILED_REPAY_WITH_COLLATERAL") + + }); + it('User 2 tries to repay his DAI Variable loan using his WETH collateral. First half the amount, after that, the rest', async () => { const {pool, weth, dai, users, mockSwapAdapter, oracle} = testEnv; const user = users[1]; const amountToRepay = parseEther('10'); + await waitForTx(await mockSwapAdapter.setTryReentrancy(false)) + const {userData: wethUserDataBefore} = await getContractsData( weth.address, user.address,