From ee72ff928a231f5f5f8f7a5e23922c48c7a8e9a6 Mon Sep 17 00:00:00 2001 From: halaprix Date: Wed, 22 Nov 2023 14:00:06 +0100 Subject: [PATCH] chore: address review comments --- .../commands/AaveV3BasicBuyCommandV2.sol | 42 ++++++++------ .../commands/AaveV3BasicSellCommandV2.sol | 56 +++++++++++-------- contracts/commands/BaseDMACommand.sol | 45 ++++++++++++--- 3 files changed, 93 insertions(+), 50 deletions(-) diff --git a/contracts/commands/AaveV3BasicBuyCommandV2.sol b/contracts/commands/AaveV3BasicBuyCommandV2.sol index 137630e..1fe49b1 100644 --- a/contracts/commands/AaveV3BasicBuyCommandV2.sol +++ b/contracts/commands/AaveV3BasicBuyCommandV2.sol @@ -29,17 +29,19 @@ import { IOperationExecutor, Call } from "../interfaces/IOperationExecutor.sol"; import { BaseDMACommand, ICommand } from "./BaseDMACommand.sol"; struct BasicBuyTriggerData { - address positionAddress; - uint16 triggerType; - uint256 maxCoverage; - address debtToken; - address collateralToken; - bytes32 operationHash; - uint256 executionLTV; - uint256 targetLTV; - uint256 maxBuyPrice; - uint64 deviation; - uint32 maxBaseFeeInGwei; + /* start of common V2 TriggerData parameters */ + address positionAddress; // Address of the position - dpm proxy + uint16 triggerType; // Type of trigger + uint256 maxCoverage; // Maximum coverage amount - max amount of additional debt taken to cover execution gas fee + address debtToken; // Address of the debt token + address collateralToken; // Address of the collateral token + bytes32 operationHash; // Hash of the operation execution operation TODO + /* end of common V2 TriggerData parameters */ + uint256 executionLTV; // Execution loan-to-value ratio + uint256 targetLTV; // Target loan-to-value ratio + uint256 maxBuyPrice; // maximum buy price + uint64 deviation; // Deviation from target LTV after execution - eg 50 corresponds to 0.5% + uint32 maxBaseFeeInGwei; // Maximum base fee in Gwei } /** @@ -80,7 +82,6 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { revert EmptyAddress("price oracle"); } priceOracle = IPriceOracleGetter(priceOracleAddress); - self = address(this); } /** @@ -118,12 +119,12 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { triggerData, (BasicBuyTriggerData) ); - validateTriggerType(basicBuyTriggerData.triggerType, AAVE_V3_BASIC_BUY_TRIGGER_TYPE); + _validateTriggerType(basicBuyTriggerData.triggerType, AAVE_V3_BASIC_BUY_TRIGGER_TYPE); AaveV3BasicBuyCommandV2(self).validateoperationHash( executionData, basicBuyTriggerData.operationHash ); - validateSelector(operationExecutor.executeOp.selector, executionData); + _validateSelector(operationExecutor.executeOp.selector, executionData); IAccountImplementation(basicBuyTriggerData.positionAddress).execute( address(operationExecutor), executionData @@ -134,7 +135,7 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { * @inheritdoc ICommand */ function isTriggerDataValid( - bool omitted, + bool, bytes memory triggerData ) external view override returns (bool) { BasicBuyTriggerData memory basicBuyTriggerData = abi.decode( @@ -154,11 +155,14 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { // assure that the execution LTV is lower than the lower target, so it wont execute again bool executionLtvBelowLowerTarget = basicBuyTriggerData.executionLTV < lowerBoundTarget; // assure that the trigger type is the correct one - bool triggerTypeCorrect = basicBuyTriggerData.triggerType == AAVE_V3_BASIC_BUY_TRIGGER_TYPE; + bool triggerTypeCorrect = _isTriggerTypeValid( + basicBuyTriggerData.triggerType, + AAVE_V3_BASIC_BUY_TRIGGER_TYPE + ); // assure that the upper bound of target LTV is lower than the max LTV, it would revert on execution bool upperTargetBelowMaxLtv = upperBoundTarget < maxLtv; // assure that the deviation is valid ( above minimal allowed deviation) - bool deviationValid = deviationIsValid(basicBuyTriggerData.deviation); + bool deviationValid = _isDeviationValid(basicBuyTriggerData.deviation); return executionLtvBelowLowerTarget && triggerTypeCorrect && @@ -181,6 +185,8 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { (uint256 totalCollateralBase, uint256 totalDebtBase, , , uint256 maxLtv, ) = lendingPool .getUserAccountData(basicBuyTriggerData.positionAddress); + /* if there is no debt or colalteral we skip the checks - it will happen eg if the user closed + the position but havent removed the trigger */ if (totalCollateralBase == 0 || totalDebtBase == 0) { return false; } @@ -203,7 +209,7 @@ contract AaveV3BasicBuyCommandV2 is BaseDMACommand { bool priceBelowMax = currentPrice <= basicBuyTriggerData.maxBuyPrice; // blocks base fee has to be below the limit set by the user (maxBaseFeeInGwei) - bool baseFeeValid = baseFeeIsValid(basicBuyTriggerData.maxBaseFeeInGwei); + bool baseFeeValid = _isBaseFeeValid(basicBuyTriggerData.maxBaseFeeInGwei); // upper bound of target LTV after execution has to be below the max LTV // it is checked again as maxLtv might have changed since the trigger was created diff --git a/contracts/commands/AaveV3BasicSellCommandV2.sol b/contracts/commands/AaveV3BasicSellCommandV2.sol index 133d00c..2ce1c9a 100644 --- a/contracts/commands/AaveV3BasicSellCommandV2.sol +++ b/contracts/commands/AaveV3BasicSellCommandV2.sol @@ -28,18 +28,23 @@ import { IAccountImplementation } from "../interfaces/IAccountImplementation.sol import { IOperationExecutor, Call } from "../interfaces/IOperationExecutor.sol"; import { BaseDMACommand, ICommand } from "./BaseDMACommand.sol"; +/** + * @dev Struct representing the data for a basic sell trigger. + */ struct BasicSellTriggerData { - address positionAddress; - uint16 triggerType; - uint256 maxCoverage; - address debtToken; - address collateralToken; - bytes32 operationHash; - uint256 execLTV; - uint256 targetLTV; - uint256 minSellPrice; - uint64 deviation; - uint32 maxBaseFeeInGwei; + /* start of common V2 TriggerData parameters */ + address positionAddress; // Address of the position - dpm proxy + uint16 triggerType; // Type of trigger + uint256 maxCoverage; // Maximum coverage amount - max amount of additional debt taken to cover execution gas fee + address debtToken; // Address of the debt token + address collateralToken; // Address of the collateral token + bytes32 operationHash; // Hash of the operation execution operation TODO + /* end of common V2 TriggerData parameters */ + uint256 executionLtv; // Execution loan-to-value ratio + uint256 targetLTV; // Target loan-to-value ratio + uint256 minSellPrice; // Minimum sell price + uint64 deviation; // Deviation from target LTV after execution - eg 50 corresponds to 0.5% + uint32 maxBaseFeeInGwei; // Maximum base fee in Gwei } contract AaveV3BasicSellCommandV2 is BaseDMACommand { @@ -75,7 +80,6 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { revert EmptyAddress("price oracle"); } priceOracle = IPriceOracleGetter(priceOracleAddress); - self = address(this); } /** @@ -90,6 +94,10 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { (uint256 totalCollateralBase, uint256 totalDebtBase, , , , ) = lendingPool .getUserAccountData(basicSellTriggerData.positionAddress); + /* Calculate the loan-to-value (LTV) ratio for Aave V3 + LTV is the ratio of the total debt to the total collateral, expressed as a percentage + The result is multiplied by 10000 to preserve precision + eg 0.67 (67%) LTV is stored as 6700 */ uint256 ltv = (totalDebtBase * 10000) / totalCollateralBase; (uint256 lowerBoundTarget, uint256 upperBoundTarget) = basicSellTriggerData @@ -113,12 +121,12 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { triggerData, (BasicSellTriggerData) ); - validateTriggerType(basicSellTriggerData.triggerType, AAVE_V3_BASIC_SELL_TRIGGER_TYPE); + _validateTriggerType(basicSellTriggerData.triggerType, AAVE_V3_BASIC_SELL_TRIGGER_TYPE); AaveV3BasicSellCommandV2(self).validateoperationHash( executionData, basicSellTriggerData.operationHash ); - validateSelector(operationExecutor.executeOp.selector, executionData); + _validateSelector(operationExecutor.executeOp.selector, executionData); IAccountImplementation(basicSellTriggerData.positionAddress).execute( address(operationExecutor), executionData @@ -129,7 +137,7 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { * @inheritdoc ICommand */ function isTriggerDataValid( - bool omitted, + bool, bytes memory triggerData ) external view override returns (bool) { BasicSellTriggerData memory basicSellTriggerData = abi.decode( @@ -144,14 +152,16 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { ); // assure that the execution LTV is higher or equal than the upper bound target, so it wont execute again - bool executionLtvAboveUpperTarget = basicSellTriggerData.execLTV >= upperBoundTarget; + bool executionLtvAboveUpperTarget = basicSellTriggerData.executionLtv >= upperBoundTarget; // assure that the trigger type is the correct one - bool triggerTypeCorrect = basicSellTriggerData.triggerType == - AAVE_V3_BASIC_SELL_TRIGGER_TYPE; + bool triggerTypeCorrect = _isTriggerTypeValid( + basicSellTriggerData.triggerType, + AAVE_V3_BASIC_SELL_TRIGGER_TYPE + ); // assure the execution LTV is lower than max LTV - bool executionLtvBelowMaxLtv = basicSellTriggerData.execLTV < maxLtv; + bool executionLtvBelowMaxLtv = basicSellTriggerData.executionLtv < maxLtv; // assure that the deviation is valid ( above minimal allowe deviation) - bool deviationValid = deviationIsValid(basicSellTriggerData.deviation); + bool deviationValid = _isDeviationValid(basicSellTriggerData.deviation); return executionLtvAboveUpperTarget && triggerTypeCorrect && @@ -186,16 +196,16 @@ contract AaveV3BasicSellCommandV2 is BaseDMACommand { uint256 currentPrice = priceOracle.getAssetPrice(basicSellTriggerData.collateralToken); // LTV has to be below or equal the execution LTV set by the user to execute - bool ltvAboveExecution = ltv >= basicSellTriggerData.execLTV; + bool ltvAboveExecution = ltv >= basicSellTriggerData.executionLtv; // oracle price has to be above the minSellPirce set by the user bool priceAboveMin = currentPrice >= basicSellTriggerData.minSellPrice; // blocks base fee has to be below the limit set by the user (maxBaseFeeInGwei) - bool baseFeeValid = baseFeeIsValid(basicSellTriggerData.maxBaseFeeInGwei); + bool baseFeeValid = _isBaseFeeValid(basicSellTriggerData.maxBaseFeeInGwei); // is execution LTV lower than max LTV - we revert early if that's not true - bool executionLtvBelowMaxLtv = basicSellTriggerData.execLTV < maxLtv; + bool executionLtvBelowMaxLtv = basicSellTriggerData.executionLtv < maxLtv; return ltvAboveExecution && priceAboveMin && baseFeeValid && executionLtvBelowMaxLtv; } diff --git a/contracts/commands/BaseDMACommand.sol b/contracts/commands/BaseDMACommand.sol index eafb44d..a2ad353 100644 --- a/contracts/commands/BaseDMACommand.sol +++ b/contracts/commands/BaseDMACommand.sol @@ -82,6 +82,7 @@ abstract contract BaseDMACommand is ReentrancyGuard, ICommand { if (weth == address(0)) { revert EmptyAddress("weth"); } + self = address(this); } /** @@ -89,7 +90,7 @@ abstract contract BaseDMACommand is ReentrancyGuard, ICommand { * @param deviation The deviation value to check. * @return A boolean indicating whether the deviation is valid or not. */ - function deviationIsValid(uint256 deviation) internal pure returns (bool) { + function _isDeviationValid(uint256 deviation) internal pure returns (bool) { return deviation >= MIN_ALLOWED_DEVIATION; } @@ -98,33 +99,59 @@ abstract contract BaseDMACommand is ReentrancyGuard, ICommand { * @param maxAcceptableBaseFeeInGwei The maximum acceptable base fee in Gwei. * @return A boolean indicating whether the base fee is valid or not. */ - function baseFeeIsValid(uint256 maxAcceptableBaseFeeInGwei) internal view returns (bool) { - return block.basefee <= maxAcceptableBaseFeeInGwei * (10 ** 9); + function _isBaseFeeValid(uint256 maxAcceptableBaseFeeInGwei) internal view returns (bool) { + return block.basefee <= maxAcceptableBaseFeeInGwei * 1 gwei; } /** - * @dev Validates the trigger type. + * @dev Validates the trigger type. Reverts if that's not the case * @param triggerType The actual trigger type. * @param expectedTriggerType The expected trigger type. */ - function validateTriggerType(uint16 triggerType, uint16 expectedTriggerType) internal pure { - if (triggerType != expectedTriggerType) { + function _validateTriggerType(uint16 triggerType, uint16 expectedTriggerType) internal pure { + if (!_isTriggerTypeValid(triggerType, expectedTriggerType)) { revert InvalidTriggerType(triggerType); } } /** - * @dev Validates the selector. + * @dev Checks if the given trigger type is valid. + * @param triggerType The trigger type to check. + * @param expectedTriggerType The expected trigger type. + * @return A boolean indicating whether the trigger type is valid or not. + */ + function _isTriggerTypeValid( + uint16 triggerType, + uint16 expectedTriggerType + ) internal pure returns (bool) { + return triggerType != expectedTriggerType; + } + + /** + * @dev Validates the selector. Reverts in case it is not. * @param expectedSelector The expected selector. * @param executionData The execution data containing the selector. */ - function validateSelector(bytes4 expectedSelector, bytes memory executionData) public pure { + function _validateSelector(bytes4 expectedSelector, bytes memory executionData) internal pure { bytes4 selector = abi.decode(executionData, (bytes4)); - if (selector != expectedSelector) { + if (_isSelectorValid(expectedSelector, selector)) { revert InvalidSelector(selector); } } + /** + * @dev Checks if the given selector is valid by comparing it with the expected selector. + * @param expectedSelector The expected selector. + * @param selector The selector to be checked. + * @return A boolean indicating whether the selector is valid or not. + */ + function _isSelectorValid( + bytes4 expectedSelector, + bytes4 selector + ) internal pure returns (bool) { + return selector != expectedSelector; + } + /** * @dev Validates the operation hash. * @param _data The operation executor execution data containing the operation hash.