Gelato Aave Security Assessment (DONE)

# Gelato Aave Security Assessment (DONE)   # Findings ## Incorrect calculation of amtOfColToWithdraw and amtToFlashBorrow  https://github.com/gelatodigital/gelato-aave/blob/4a6d42fc61e21ca1e0850d6d088763e2cd08d12d/contracts/functions/FProtectionResolver.sol#L18-L66 ```solidity= function _amountToPaybackAndWithdraw( bytes32 _id, ProtectionDataCompute memory _protectionDataCompute ) view returns (RepayAndWithdrawResult memory) { // Liquidation is a 4 digit values lower than 10000. uint256 intermediateValue = _wdiv( ( ( (_wmul( _protectionDataCompute.wantedHealthFactor, _protectionDataCompute.totalBorrowsETH ) - ( _qmul( _protectionDataCompute.totalCollateralETH, _protectionDataCompute .currentLiquidationThreshold ) )) ) ), _qmul( _protectionDataCompute.wantedHealthFactor, (TEN_THOUSAND_BPS - _protectionDataCompute.flashloanPremium) ) - _qmul( _protectionDataCompute.colLiquidationThreshold, (TEN_THOUSAND_BPS + _protectionDataCompute.protectionActionFee) ) * 1e14 ); uint256 colTokenDecimals = ERC20(_protectionDataCompute.colToken).decimals(); uint256 debtTokenDecimals = ERC20(_protectionDataCompute.debtToken).decimals(); return RepayAndWithdrawResult( _id, ((_tokenToTokenPrecision(intermediateValue, 18, colTokenDecimals) * 10**colTokenDecimals) / _protectionDataCompute.colPrice), ((_tokenToTokenPrecision(intermediateValue, 18, debtTokenDecimals) * 10**debtTokenDecimals) / _protectionDataCompute.debtPrice), "OK" ); } ``` ### Issue Description $amtOfColToWithdraw = amtToFlashBorrow \times (1 + flashloanPremiumRate)$ $wantedHealthFactor = {totalCollateralETH \times currentLiquidationThreshold - amtOfColToWithdraw \times (1 + protectionActionFeeRate) \times colLiquidationThreshold \over totalBorrowsETH - amtToFlashBorrow}$ $\iff$ $amtOfColToWithdraw = { wantedHealthFactor \times totalBorrowsETH - totalCollateralETH \times currentLiquidationThreshold \over {wantedHealthFactor \over 1 + flashloanPremiumRate} - (1 + protectionActionFeeRate) \times colLiquidationThreshold}$ $amtToFlashBorrow = {amtOfColToWithdraw \over 1 + flashloanPremiumRate}$ ### Recommendation - change ```solidity= _qmul( _protectionDataCompute.wantedHealthFactor, (TEN_THOUSAND_BPS - _protectionDataCompute.flashloanPremium) ) ``` to ```solidity= _qdiv( _protectionDataCompute.wantedHealthFactor, (TEN_THOUSAND_BPS + _protectionDataCompute.flashloanPremium) ) ``` - change ```solidity= return RepayAndWithdrawResult( _id, ((_tokenToTokenPrecision(intermediateValue, 18, colTokenDecimals) * 10**colTokenDecimals) / _protectionDataCompute.colPrice), ((_tokenToTokenPrecision(intermediateValue, 18, debtTokenDecimals) * 10**debtTokenDecimals) / _protectionDataCompute.debtPrice), "OK" ); ``` to ```solidity= return RepayAndWithdrawResult( _id, ((_tokenToTokenPrecision(intermediateValue, 18, colTokenDecimals) * 10**colTokenDecimals) / _protectionDataCompute.colPrice), ((_tokenToTokenPrecision(_qdiv(intermediateValue, TEN_THOUSAND_BPS + _protectionDataCompute.flashloanPremium), 18, debtTokenDecimals) * 10**debtTokenDecimals) / _protectionDataCompute.debtPrice), "OK" ); ```  --- ## Miscalculation in _tokenToTokenPrecision when _oldPrecision <= _newPrecision  https://github.com/gelatodigital/gelato-aave/blob/4a6d42fc61e21ca1e0850d6d088763e2cd08d12d/contracts/functions/FProtectionResolver.sol#L122-L131 ```solidity= function _tokenToTokenPrecision( uint256 _amount, uint256 _oldPrecision, uint256 _newPrecision ) pure returns (uint256) { return _oldPrecision > _newPrecision ? _amount / (10**(_oldPrecision - _newPrecision)) : _amount * (10**(_oldPrecision - _newPrecision)); } ``` ### Issue Description When `_oldPrecision <= _newPrecision`, it should return `_amount * (10**(_newPrecision - _oldPrecision))` ### Recommendation change ```solidity= _amount * (10**(_oldPrecision - _newPrecision)) ``` to ```solidity= _amount * (10**(_newPrecision - _oldPrecision)) ``` --- ## Unused code  ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/master/contracts/services/aave/resolver/ProtectionResolver.sol#L131-L141 https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionResolver.sol#L83-L118 The variable `colInETH` and `debtInETH` in `ProtectionResolver.sol#protectionDataCompute()` and the function `_getColInETH` and `_getDebtInEth` in `FProtectionResolver.sol` are unused. They may be leftover from a previous version. ### Recommendation Consider removing unused variables and functions. --- ## Misleading names  ### Issue Description - https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionAction.sol#L107 The second parameter of `_withdrawCollateral()` named `aaveService` is actually not the aaveService contract. **Recommendation**: Consider changing the name to `recipient` or `to`. - https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionAction.sol#L144 The function `FProtectionAction.sol#_isPositionSafe` and `FProtectionAction.sol#_isPositionUnSafe` suggests that it returns a bool that indicates whether the position is safe/unsafe, while it actually requires the position to be safe/unsafe. https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionResolver.sol#L139-L146 Using `_isPositionSafe` and `_isPositionUnSafe` is also likely to be confused with `FProtectionResolver.sol#_isPositionUnsafe`. **Recommendation**: Consider changing the name from `_isPositionUnSafe` to `_requirePositionUnSafe`, and `_isPositionSafe` to `_requirePositionSafe`. - https://github.com/gelatodigital/gelato-aave/blob/e9cd8e37aea7117273decc379175cfe923f780a6/contracts/services/aave/resolver/ProtectionResolver.sol#L129-L132 The name `protectionActionFee` and `flashloanPremium` might be misinterpreted as the amount of the fees. **Recommendation**: Consider changing the name from `protectionActionFee` to `protectionActionFeeBps`, and `flashloanPremium` to `flashloanPremiumBps`. --- ## Unnecessary imports  ### Issue Description - https://github.com/gelatodigital/gelato-aave/blob/master/contracts/services/aave/FlashLoanReceiverBase.sol#L17 Unnecessary import of `SafeMath` library in `FlashLoanReceiverBase.sol`. - https://github.com/gelatodigital/gelato-aave/blob/master/contracts/structs/SProtection.sol#L4 `import {Path} from "../structs/SParaswap.sol"` - https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionAction.sol#L10 `import {_rdiv} from "../vendor/DSMath.sol"` - https://github.com/gelatodigital/gelato-aave/blob/7bc98ff445dc700e158a56832e35a677e2313fc2/contracts/services/aave/resolver/ProtectionResolver.sol#L12 Unnecessary import of `IPriceOracle`, `ProtectionDataCompute` and `IERC20`. ### Recommendation Remove unnecessary imports. --- ## Wrong name or value of `isPositionSafe`  ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/master/contracts/services/aave/resolver/ProtectionResolver.sol#L188-L191 ```solidity= result.isPositionSafe = _isPositionUnsafe( _canExecData.user, _canExecData.minimumHF ); ``` ### Recommendation Consider changing to: ```solidity= result.isPositionSafe = !_isPositionUnsafe( _canExecData.user, _canExecData.minimumHF ); ``` or ```solidity= result.isPositionUnsafe = _isPositionUnsafe( _canExecData.user, _canExecData.minimumHF ); ``` --- ## Unnecessary ternary statements ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/master/contracts/functions/FProtectionResolver.sol#L94-L114 ```solidity= function _isPositionUnsafe(address _user, uint256 _minimumHF) view returns (bool) { (, , , , , uint256 currenthealthFactor) = ILendingPool(LENDINGPOOL).getUserAccountData(_user); return currenthealthFactor < _minimumHF ? true : false; } function _isAllowed( address _aToken, address _user, address _spender, uint256 _allowedAmt ) view returns (bool) { return IERC20(_aToken).balanceOf(_user) >= _allowedAmt && IERC20(_aToken).allowance(_user, _spender) >= _allowedAmt ? true : false; } ``` The ternary statements in `FProtectionResolver.sol#_isPositionUnsafe()` and `FProtectionResolver.sol#_isAllowed()` are unnecessary. They may be leftover from a previous version. ### Recommendation Consider removing the unnecessary ternary statements. --- ## Leftover debtTokens in edge cases are not returned ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/master/contracts/services/aave/actions/ProtectionAction.sol#L195-L201 The current implementation of `ProtectionAction.sol#_executeOperation()` won't return the leftover debtTokens in edge cases of the `debtRepaid` being larger than the debt amount. As an example, consider the scenario of: Current debt amount: 10,000 USDC Calculated amtOfDebtToRepay : 9,999 USDC After the task is generated and submitted, but before executed, the user repaid the debt for 2 USDC. So, by the time we execute the task, the `amtOfDebtToRepay` will actually be larger than the debt amount (9,998 USDC). As a result, 1 USDC will be left in the contract. ### Recommendation Consider checking for leftover debtTokens after `_paybackToLendingPool` and send them back to the user. --- ## Using all the wallet balance can cause Denial of Service (DoS) ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/f58b6b042090582edec59c2b65f1497a5f8901c7/contracts/services/aave/actions/ProtectionAction.sol#L200 https://github.com/gelatodigital/gelato-aave/blob/7bc98ff445dc700e158a56832e35a677e2313fc2/contracts/functions/FProtectionAction.sol#L242 `ProtectionAction.sol#_executeOperation()` and `FProtectionAction.sol#_swap()` is using all the wallet balance to swap and repay debt. An attacker may send a certain amount of colToken/debtToken to the `ProtectionAction` contract which leads to over repay of the debt so that the result Health Factor can be higher than the upper limit and cause the transaction to be reverted. ### Recommendation Consider using `_flashloanData.amounts[0]` to swap, and `receivedAmt` to repay debt. --- ## ProtectionAction.sol allows for arbitrary code execution ### Issue Description https://github.com/gelatodigital/gelato-aave/blob/f58b6b042090582edec59c2b65f1497a5f8901c7/contracts/functions/FProtectionAction.sol#L181 The Gelato protocol's design allows the executors to craft calldata and make call to `ProtectionAction.sol` on behalf of the GELATO address, and the on-chain requirements only enforced the post protection health factor. A compromised or malicious executor can take advantage of this and stealing user's funds. As an example, consider the scenario of: A user with 200 ETH worth of collateral and 50 ETH worth of debt, and the weighted average liquidation threshold is 50%, then the current Health Factor will be: (200 * 0.5) / 50 = 200% If the target Health Factor of the protection task is 300%. A malicious executor can craft a transaction that do the following: 1. Withdraw collateral and repay 49.9 ETH worth of debt; 2. Withdraw 149.5 ETH worth of collateral and transfer to the executor's address; The final Health Factor will be: (200 - 49.9 - 149.5) * 0.5 / (50 - 49.9) = 300% Which will pass the onchain check. ### Recommendation Create an isolated swap contract and heighten the on-chain requirements. ### Resolution This issue has been addressed with commit [7bc98ff445dc700e158a56832e35a677e2313fc2](https://github.com/gelatodigital/gelato-aave/commit/7bc98ff445dc700e158a56832e35a677e2313fc2) by adding an isolated swap contract and on-chain verification of `amtToFlashBorrow` and `amtOfDebtToRepay`.