From b907b6b0b3ad1bedf61273d9ce38ced4c4748892 Mon Sep 17 00:00:00 2001 From: The3D Date: Wed, 30 Sep 2020 17:40:47 +0200 Subject: [PATCH 1/7] Initial fix --- contracts/libraries/helpers/Errors.sol | 4 +++- contracts/tokenization/AToken.sol | 9 ++++++--- contracts/tokenization/VariableDebtToken.sol | 12 ++++++++++-- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/contracts/libraries/helpers/Errors.sol b/contracts/libraries/helpers/Errors.sol index 48fc7f32..42d93a39 100644 --- a/contracts/libraries/helpers/Errors.sol +++ b/contracts/libraries/helpers/Errors.sol @@ -45,10 +45,12 @@ library Errors { string public constant INVALID_EQUAL_ASSETS_TO_SWAP = '56'; string public constant NO_MORE_RESERVES_ALLOWED = '59'; - // require error messages - aToken + // require error messages - aToken - DebtTokens string public constant CALLER_MUST_BE_LENDING_POOL = '28'; // 'The caller of this function must be a lending pool' string public constant CANNOT_GIVE_ALLOWANCE_TO_HIMSELF = '30'; // 'User cannot give allowance to himself' string public constant TRANSFER_AMOUNT_NOT_GT_0 = '31'; // 'Transferred amount needs to be greater than zero' + string public constant INVALID_MINT_AMOUNT = '53'; //invalid amount to mint + string public constant INVALID_BURN_AMOUNT = '53'; //invalid amount to burn // require error messages - ReserveLogic string public constant RESERVE_ALREADY_INITIALIZED = '34'; // 'Reserve has already been initialized' diff --git a/contracts/tokenization/AToken.sol b/contracts/tokenization/AToken.sol index 98c54bcd..8dcf669e 100644 --- a/contracts/tokenization/AToken.sol +++ b/contracts/tokenization/AToken.sol @@ -101,7 +101,9 @@ contract AToken is VersionedInitializable, IncentivizedERC20, IAToken { uint256 amount, uint256 index ) external override onlyLendingPool { - _burn(user, amount.rayDiv(index)); + uint256 amountScaled = amount.rayDiv(index); + require(amountScaled != 0, Errors.INVALID_BURN_AMOUNT); + _burn(user, amountScaled); //transfers the underlying to the target IERC20(UNDERLYING_ASSET_ADDRESS).safeTransfer(receiverOfUnderlying, amount); @@ -122,8 +124,9 @@ contract AToken is VersionedInitializable, IncentivizedERC20, IAToken { uint256 amount, uint256 index ) external override onlyLendingPool { - //mint an equivalent amount of tokens to cover the new deposit - _mint(user, amount.rayDiv(index)); + uint256 amountScaled = amount.rayDiv(index); + require(amountScaled != 0, Errors.INVALID_MINT_AMOUNT); + _mint(user, amountScaled); //transfer event to track balances emit Transfer(address(0), user, amount); diff --git a/contracts/tokenization/VariableDebtToken.sol b/contracts/tokenization/VariableDebtToken.sol index 5e0f2dff..e7bd7309 100644 --- a/contracts/tokenization/VariableDebtToken.sol +++ b/contracts/tokenization/VariableDebtToken.sol @@ -7,6 +7,7 @@ import {SafeMath} from '@openzeppelin/contracts/math/SafeMath.sol'; import {DebtTokenBase} from './base/DebtTokenBase.sol'; import {WadRayMath} from '../libraries/math/WadRayMath.sol'; import {IVariableDebtToken} from './interfaces/IVariableDebtToken.sol'; +import {Errors} from '../libraries/helpers/Errors.sol'; /** * @title contract VariableDebtToken @@ -60,7 +61,10 @@ contract VariableDebtToken is DebtTokenBase, IVariableDebtToken { uint256 index ) external override onlyLendingPool { - _mint(user, amount.rayDiv(index)); + uint256 amountScaled = amount.rayDiv(index); + require(amountScaled != 0, Errors.INVALID_MINT_AMOUNT); + + _mint(user, amountScaled); emit Transfer(address(0), user, amount); emit Mint(user, amount, index); @@ -76,7 +80,11 @@ contract VariableDebtToken is DebtTokenBase, IVariableDebtToken { uint256 amount, uint256 index ) external override onlyLendingPool { - _burn(user, amount.rayDiv(index)); + + uint256 amountScaled = amount.rayDiv(index); + require(amountScaled != 0, Errors.INVALID_BURN_AMOUNT); + + _burn(user, amountScaled); emit Transfer(user, address(0), amount); emit Burn(user, amount, index); From 758675c132ae74fb99b2bb293ecd193159d3fd91 Mon Sep 17 00:00:00 2001 From: The3D Date: Wed, 30 Sep 2020 17:59:47 +0200 Subject: [PATCH 2/7] Fixes tests --- .../LendingPoolCollateralManager.sol | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/contracts/lendingpool/LendingPoolCollateralManager.sol b/contracts/lendingpool/LendingPoolCollateralManager.sol index 75f4f659..bfec17cd 100644 --- a/contracts/lendingpool/LendingPoolCollateralManager.sol +++ b/contracts/lendingpool/LendingPoolCollateralManager.sol @@ -238,12 +238,15 @@ contract LendingPoolCollateralManager is VersionedInitializable, LendingPoolStor principalReserve.variableBorrowIndex ); } else { - IVariableDebtToken(principalReserve.variableDebtTokenAddress).burn( - user, - vars.userVariableDebt, - principalReserve.variableBorrowIndex - ); - + //if the user does not have variable debt, no need to try to burn variable + //debt tokens + if (vars.userVariableDebt > 0) { + IVariableDebtToken(principalReserve.variableDebtTokenAddress).burn( + user, + vars.userVariableDebt, + principalReserve.variableBorrowIndex + ); + } IStableDebtToken(principalReserve.stableDebtTokenAddress).burn( user, vars.actualAmountToLiquidate.sub(vars.userVariableDebt) @@ -412,7 +415,11 @@ contract LendingPoolCollateralManager is VersionedInitializable, LendingPoolStor vars.actualAmountToLiquidate, 0 ); - IERC20(principal).safeTransferFrom(receiver, vars.principalAToken, vars.actualAmountToLiquidate); + IERC20(principal).safeTransferFrom( + receiver, + vars.principalAToken, + vars.actualAmountToLiquidate + ); if (vars.userVariableDebt >= vars.actualAmountToLiquidate) { IVariableDebtToken(debtReserve.variableDebtTokenAddress).burn( From 5a67250743ba42cc4a1097b0fffd828a9e17714e Mon Sep 17 00:00:00 2001 From: The3D Date: Wed, 30 Sep 2020 18:03:34 +0200 Subject: [PATCH 3/7] fixed error code --- contracts/libraries/helpers/Errors.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/libraries/helpers/Errors.sol b/contracts/libraries/helpers/Errors.sol index 42d93a39..cf0923c7 100644 --- a/contracts/libraries/helpers/Errors.sol +++ b/contracts/libraries/helpers/Errors.sol @@ -50,7 +50,7 @@ library Errors { string public constant CANNOT_GIVE_ALLOWANCE_TO_HIMSELF = '30'; // 'User cannot give allowance to himself' string public constant TRANSFER_AMOUNT_NOT_GT_0 = '31'; // 'Transferred amount needs to be greater than zero' string public constant INVALID_MINT_AMOUNT = '53'; //invalid amount to mint - string public constant INVALID_BURN_AMOUNT = '53'; //invalid amount to burn + string public constant INVALID_BURN_AMOUNT = '54'; //invalid amount to burn // require error messages - ReserveLogic string public constant RESERVE_ALREADY_INITIALIZED = '34'; // 'Reserve has already been initialized' From c45d6c254bd66a2a29ac88e50ba850f926b9bd60 Mon Sep 17 00:00:00 2001 From: The3D Date: Mon, 5 Oct 2020 13:11:53 +0200 Subject: [PATCH 4/7] fixes #56 --- contracts/tokenization/AToken.sol | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contracts/tokenization/AToken.sol b/contracts/tokenization/AToken.sol index 8dcf669e..4be67c12 100644 --- a/contracts/tokenization/AToken.sol +++ b/contracts/tokenization/AToken.sol @@ -134,7 +134,17 @@ contract AToken is VersionedInitializable, IncentivizedERC20, IAToken { } function mintToTreasury(uint256 amount, uint256 index) external override onlyLendingPool { - _mint(RESERVE_TREASURY_ADDRESS, amount.div(index)); + + if(amount == 0){ + return; + } + + //compared to the normal mint, we don't check for rounding errors. + //the amount to mint can easily be very small since is a fraction of the interest + //accrued. in that case, the treasury will experience a (very small) loss, but it + //wont cause potentially valid transactions to fail. + + _mint(RESERVE_TREASURY_ADDRESS, amount.rayDiv(index)); //transfer event to track balances emit Transfer(address(0), RESERVE_TREASURY_ADDRESS, amount); From 3ad9e7b9e01c4d639bc587ff32b8eb6820880cb3 Mon Sep 17 00:00:00 2001 From: The3D Date: Mon, 5 Oct 2020 13:12:58 +0200 Subject: [PATCH 5/7] Added natspec comment to mintToTreasury --- contracts/tokenization/AToken.sol | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/contracts/tokenization/AToken.sol b/contracts/tokenization/AToken.sol index 4be67c12..016a5601 100644 --- a/contracts/tokenization/AToken.sol +++ b/contracts/tokenization/AToken.sol @@ -118,6 +118,7 @@ contract AToken is VersionedInitializable, IncentivizedERC20, IAToken { * only lending pools can call this function * @param user the address receiving the minted tokens * @param amount the amount of tokens to mint + * @param index the the last index of the reserve */ function mint( address user, @@ -133,6 +134,13 @@ contract AToken is VersionedInitializable, IncentivizedERC20, IAToken { emit Mint(user, amount, index); } + + /** + * @dev mints aTokens to reserve treasury + * only lending pools can call this function + * @param amount the amount of tokens to mint to the treasury + * @param index the the last index of the reserve + */ function mintToTreasury(uint256 amount, uint256 index) external override onlyLendingPool { if(amount == 0){ From 273070fada1a7d91631f1f3c63f61b41dbee3139 Mon Sep 17 00:00:00 2001 From: eboado Date: Wed, 7 Oct 2020 16:20:32 +0200 Subject: [PATCH 6/7] Added return uint256 validation to flashLoan() --- .../interfaces/IFlashLoanReceiver.sol | 2 +- contracts/lendingpool/LendingPool.sol | 5 ++++- contracts/libraries/helpers/Errors.sol | 1 + .../mocks/flashloan/MockFlashLoanReceiver.sol | 16 ++++++++++++-- helpers/types.ts | 1 + test/flashloan.spec.ts | 22 +++++++++++++++++++ 6 files changed, 43 insertions(+), 4 deletions(-) diff --git a/contracts/flashloan/interfaces/IFlashLoanReceiver.sol b/contracts/flashloan/interfaces/IFlashLoanReceiver.sol index e3c2636c..be25e761 100644 --- a/contracts/flashloan/interfaces/IFlashLoanReceiver.sol +++ b/contracts/flashloan/interfaces/IFlashLoanReceiver.sol @@ -13,5 +13,5 @@ interface IFlashLoanReceiver { uint256 amount, uint256 fee, bytes calldata params - ) external; + ) external returns(uint256); } diff --git a/contracts/lendingpool/LendingPool.sol b/contracts/lendingpool/LendingPool.sol index 6afb3eed..305bea8b 100644 --- a/contracts/lendingpool/LendingPool.sol +++ b/contracts/lendingpool/LendingPool.sol @@ -570,7 +570,10 @@ contract LendingPool is VersionedInitializable, ILendingPool, LendingPoolStorage IAToken(vars.aTokenAddress).transferUnderlyingTo(receiverAddress, amount); //execute action of the receiver - vars.receiver.executeOperation(asset, amount, vars.premium, params); + require( + vars.receiver.executeOperation(asset, amount, vars.premium, params) != 0, + Errors.INVALID_FLASH_LOAN_EXECUTOR_RETURN + ); vars.amountPlusPremium = amount.add(vars.premium); diff --git a/contracts/libraries/helpers/Errors.sol b/contracts/libraries/helpers/Errors.sol index 48fc7f32..97c4bf12 100644 --- a/contracts/libraries/helpers/Errors.sol +++ b/contracts/libraries/helpers/Errors.sol @@ -44,6 +44,7 @@ library Errors { string public constant FAILED_COLLATERAL_SWAP = '55'; string public constant INVALID_EQUAL_ASSETS_TO_SWAP = '56'; string public constant NO_MORE_RESERVES_ALLOWED = '59'; + string public constant INVALID_FLASH_LOAN_EXECUTOR_RETURN = '60'; // require error messages - aToken string public constant CALLER_MUST_BE_LENDING_POOL = '28'; // 'The caller of this function must be a lending pool' diff --git a/contracts/mocks/flashloan/MockFlashLoanReceiver.sol b/contracts/mocks/flashloan/MockFlashLoanReceiver.sol index 112084a7..4a4ce463 100644 --- a/contracts/mocks/flashloan/MockFlashLoanReceiver.sol +++ b/contracts/mocks/flashloan/MockFlashLoanReceiver.sol @@ -20,6 +20,7 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { bool _failExecution; uint256 _amountToApprove; + bool _simulateEOA; constructor(ILendingPoolAddressesProvider provider) public FlashLoanReceiverBase(provider) {} @@ -31,16 +32,25 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { _amountToApprove = amountToApprove; } + function setSimulateEOA(bool flag) public { + _simulateEOA = flag; + } + function amountToApprove() public view returns (uint256) { return _amountToApprove; } + function simulateEOA() public view returns(bool) { + return _simulateEOA; + } + function executeOperation( address reserve, uint256 amount, uint256 fee, bytes memory params - ) public override { + ) public override returns(uint256) { + params; //mint to this contract the specific amount MintableERC20 token = MintableERC20(reserve); @@ -51,7 +61,7 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { if (_failExecution) { emit ExecutedWithFail(reserve, amount, fee); - return; + return (_simulateEOA) ? 0 : amountToReturn; } //execution does not fail - mint tokens and return them to the _destination @@ -62,5 +72,7 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { IERC20(reserve).approve(_addressesProvider.getLendingPool(), amountToReturn); emit ExecutedWithSuccess(reserve, amount, fee); + + return amountToReturn; } } diff --git a/helpers/types.ts b/helpers/types.ts index a8d963f0..7ed1082a 100644 --- a/helpers/types.ts +++ b/helpers/types.ts @@ -77,6 +77,7 @@ export enum ProtocolErrors { REQUESTED_AMOUNT_TOO_SMALL = '25', // 'The requested amount is too small for a FlashLoan.' INCONSISTENT_PROTOCOL_ACTUAL_BALANCE = '26', // 'The actual balance of the protocol is inconsistent' CALLER_NOT_LENDING_POOL_CONFIGURATOR = '27', // 'The actual balance of the protocol is inconsistent' + INVALID_FLASH_LOAN_EXECUTOR_RETURN = '60', // The flash loan received returned 0 (EOA) // require error messages - aToken CALLER_MUST_BE_LENDING_POOL = '28', // 'The caller of this function must be a lending pool' diff --git a/test/flashloan.spec.ts b/test/flashloan.spec.ts index c2be90ed..b82b6433 100644 --- a/test/flashloan.spec.ts +++ b/test/flashloan.spec.ts @@ -24,6 +24,7 @@ makeSuite('LendingPool FlashLoan function', (testEnv: TestEnv) => { INVALID_FLASHLOAN_MODE, SAFEERC20_LOWLEVEL_CALL, IS_PAUSED, + INVALID_FLASH_LOAN_EXECUTOR_RETURN } = ProtocolErrors; before(async () => { @@ -116,9 +117,30 @@ makeSuite('LendingPool FlashLoan function', (testEnv: TestEnv) => { ).to.be.revertedWith(TRANSFER_AMOUNT_EXCEEDS_BALANCE); }); + it('Takes WETH flashloan, simulating a receiver as EOA (revert expected)', async () => { + const {pool, weth, users} = testEnv; + const caller = users[1]; + await _mockFlashLoanReceiver.setFailExecutionTransfer(true); + await _mockFlashLoanReceiver.setSimulateEOA(true) + + await expect( + pool + .connect(caller.signer) + .flashLoan( + _mockFlashLoanReceiver.address, + weth.address, + ethers.utils.parseEther('0.8'), + 0, + '0x10', + '0' + ) + ).to.be.revertedWith(INVALID_FLASH_LOAN_EXECUTOR_RETURN); + }); + it('Takes a WETH flashloan with an invalid mode. (revert expected)', async () => { const {pool, weth, users} = testEnv; const caller = users[1]; + await _mockFlashLoanReceiver.setSimulateEOA(false) await _mockFlashLoanReceiver.setFailExecutionTransfer(true); await expect( From 80bdb0f2b206db17e9e649a5a1bc61f761acfa78 Mon Sep 17 00:00:00 2001 From: eboado Date: Mon, 12 Oct 2020 12:00:49 +0200 Subject: [PATCH 7/7] Changed return of executeOperation() to bool, as the amount is never used. --- contracts/flashloan/interfaces/IFlashLoanReceiver.sol | 2 +- contracts/lendingpool/LendingPool.sol | 2 +- contracts/mocks/flashloan/MockFlashLoanReceiver.sol | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/flashloan/interfaces/IFlashLoanReceiver.sol b/contracts/flashloan/interfaces/IFlashLoanReceiver.sol index be25e761..68292aa1 100644 --- a/contracts/flashloan/interfaces/IFlashLoanReceiver.sol +++ b/contracts/flashloan/interfaces/IFlashLoanReceiver.sol @@ -13,5 +13,5 @@ interface IFlashLoanReceiver { uint256 amount, uint256 fee, bytes calldata params - ) external returns(uint256); + ) external returns(bool); } diff --git a/contracts/lendingpool/LendingPool.sol b/contracts/lendingpool/LendingPool.sol index 305bea8b..69b6c3fa 100644 --- a/contracts/lendingpool/LendingPool.sol +++ b/contracts/lendingpool/LendingPool.sol @@ -571,7 +571,7 @@ contract LendingPool is VersionedInitializable, ILendingPool, LendingPoolStorage //execute action of the receiver require( - vars.receiver.executeOperation(asset, amount, vars.premium, params) != 0, + vars.receiver.executeOperation(asset, amount, vars.premium, params), Errors.INVALID_FLASH_LOAN_EXECUTOR_RETURN ); diff --git a/contracts/mocks/flashloan/MockFlashLoanReceiver.sol b/contracts/mocks/flashloan/MockFlashLoanReceiver.sol index 4a4ce463..77e4d5a4 100644 --- a/contracts/mocks/flashloan/MockFlashLoanReceiver.sol +++ b/contracts/mocks/flashloan/MockFlashLoanReceiver.sol @@ -49,7 +49,7 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { uint256 amount, uint256 fee, bytes memory params - ) public override returns(uint256) { + ) public override returns(bool) { params; //mint to this contract the specific amount MintableERC20 token = MintableERC20(reserve); @@ -61,7 +61,7 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { if (_failExecution) { emit ExecutedWithFail(reserve, amount, fee); - return (_simulateEOA) ? 0 : amountToReturn; + return !_simulateEOA; } //execution does not fail - mint tokens and return them to the _destination @@ -73,6 +73,6 @@ contract MockFlashLoanReceiver is FlashLoanReceiverBase { emit ExecutedWithSuccess(reserve, amount, fee); - return amountToReturn; + return true; } }