From caa71a180a6945703ca66cf5fccc33e8b9575eef Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 09:40:11 -0600 Subject: [PATCH 1/6] fix: tf remove --- src/Bases/HealthCheck/BaseHealthCheck.sol | 6 ++++-- src/swappers/TradeFactorySwapper.sol | 6 +++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Bases/HealthCheck/BaseHealthCheck.sol b/src/Bases/HealthCheck/BaseHealthCheck.sol index 1f00f81..67d592c 100644 --- a/src/Bases/HealthCheck/BaseHealthCheck.sol +++ b/src/Bases/HealthCheck/BaseHealthCheck.sol @@ -145,13 +145,15 @@ abstract contract BaseHealthCheck is BaseStrategy { if (_newTotalAssets > currentTotalAssets) { require( ((_newTotalAssets - currentTotalAssets) <= - (currentTotalAssets * uint256(_profitLimitRatio)) / MAX_BPS), + (currentTotalAssets * uint256(_profitLimitRatio)) / + MAX_BPS), "healthCheck" ); } else if (currentTotalAssets > _newTotalAssets) { require( (currentTotalAssets - _newTotalAssets <= - ((currentTotalAssets * uint256(_lossLimitRatio)) / MAX_BPS)), + ((currentTotalAssets * uint256(_lossLimitRatio)) / + MAX_BPS)), "healthCheck" ); } diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index ae533ba..598f8a3 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -51,7 +51,8 @@ abstract contract TradeFactorySwapper { ERC20(_tokenFrom).safeApprove(_tradeFactory, 0); ITradeFactory(_tradeFactory).disable(_tokenFrom, _tokenTo); - _rewardTokens = _rewardTokens; + // Set to storage + _rewardTokens = rewardTokens_; _rewardTokens.pop(); } } @@ -70,6 +71,9 @@ abstract contract TradeFactorySwapper { _removeTradeFactoryPermissions(); } + // If setting to address(0) we are done. + if (tradeFactory_ == address(0)) return; + address[] memory rewardTokens_ = _rewardTokens; ITradeFactory tf = ITradeFactory(tradeFactory_); From c44e9c07ff38d92e05c9a19a76e1d634d4e60727 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 10:01:37 -0600 Subject: [PATCH 2/6] fix: fix local naming --- src/swappers/TradeFactorySwapper.sol | 51 +++++++++++++++------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index 598f8a3..b1a1765 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -30,29 +30,30 @@ abstract contract TradeFactorySwapper { } function _addToken(address _tokenFrom, address _tokenTo) internal { - address tradeFactory_ = _tradeFactory; - if (tradeFactory_ != address(0)) { - ERC20(_tokenFrom).safeApprove(tradeFactory_, type(uint256).max); + address _tf = tradeFactory(); + if (_tf != address(0)) { + ERC20(_tokenFrom).safeApprove(_tf, type(uint256).max); - ITradeFactory(tradeFactory_).enable(_tokenFrom, _tokenTo); + ITradeFactory(_tf).enable(_tokenFrom, _tokenTo); } _rewardTokens.push(_tokenFrom); } function _removeToken(address _tokenFrom, address _tokenTo) internal { - address[] memory rewardTokens_ = _rewardTokens; - for (uint256 i; i < rewardTokens_.length; ++i) { - if (rewardTokens_[i] == _tokenFrom) { - if (i != rewardTokens_.length - 1) { + address[] memory _rewardTokensLocal = rewardTokens(); + address _tf = tradeFactory(); + for (uint256 i; i < _rewardTokensLocal.length; ++i) { + if (_rewardTokensLocal[i] == _tokenFrom) { + if (i != _rewardTokensLocal.length - 1) { // if it isn't the last token, swap with the last one/ - rewardTokens_[i] = rewardTokens_[rewardTokens_.length - 1]; + _rewardTokensLocal[i] = _rewardTokensLocal[_rewardTokensLocal.length - 1]; } - ERC20(_tokenFrom).safeApprove(_tradeFactory, 0); - ITradeFactory(_tradeFactory).disable(_tokenFrom, _tokenTo); + ERC20(_tokenFrom).safeApprove(_tf, 0); + ITradeFactory(_tf).disable(_tokenFrom, _tokenTo); // Set to storage - _rewardTokens = rewardTokens_; + _rewardTokens = _rewardTokensLocal; _rewardTokens.pop(); } } @@ -66,34 +67,36 @@ abstract contract TradeFactorySwapper { function _setTradeFactory( address tradeFactory_, address _tokenTo - ) internal { - if (_tradeFactory != address(0)) { + ) internal { + address _tf = tradeFactory(); + + // Remove any old Trade Factory + if (_tf != address(0)) { _removeTradeFactoryPermissions(); } // If setting to address(0) we are done. if (tradeFactory_ == address(0)) return; - address[] memory rewardTokens_ = _rewardTokens; - ITradeFactory tf = ITradeFactory(tradeFactory_); + address[] memory _rewardTokensLocal = _rewardTokens; // TODO: Dont iterate over the array twice - for (uint256 i; i < rewardTokens_.length; ++i) { - address token = rewardTokens_[i]; + for (uint256 i; i < _rewardTokensLocal.length; ++i) { + address token = _rewardTokensLocal[i]; - ERC20(token).safeApprove(tradeFactory_, type(uint256).max); + ERC20(token).safeApprove(_tf, type(uint256).max); - tf.enable(token, _tokenTo); + ITradeFactory(_tf).enable(token, _tokenTo); } _tradeFactory = tradeFactory_; } function _removeTradeFactoryPermissions() internal { - address[] memory rewardTokens_ = _rewardTokens; - for (uint256 i; i < rewardTokens_.length; ++i) { - ERC20(rewardTokens_[i]).safeApprove(_tradeFactory, 0); - // TODO: Add a disable + address[] memory rewardTokensLocal = rewardTokens(); + address _tf = tradeFactory(); + for (uint256 i; i < rewardTokensLocal.length; ++i) { + ERC20(rewardTokensLocal[i]).safeApprove(_tf, 0); } _tradeFactory = address(0); From 8850b3d1d6f2b9efcb169be478c9e93a08715d27 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 10:16:50 -0600 Subject: [PATCH 3/6] chore: add comments --- src/swappers/TradeFactorySwapper.sol | 66 +++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 10 deletions(-) diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index b1a1765..b2085cc 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -6,29 +6,54 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol import {ITradeFactory} from "../interfaces/TradeFactory/ITradeFactory.sol"; +/** + * @title Trade Factory Swapper + * @dev Inherit to use a Trade Factory for token swapping. + * External functions with the proper modifiers should be + * declared in the strategy that inherits this to add a + * Trade Factory and the tokens to sell. + */ abstract contract TradeFactorySwapper { using SafeERC20 for ERC20; + // Address of the trade factory in use if any. address private _tradeFactory; + // Array of any tokens added to be sold. address[] private _rewardTokens; - // We use a getter so trade factory can only be set through the - // proper functions to avoid issues. + /** + * @notice Get the current Trade Factory. + * @dev We use a getter so trade factory can only be set through the + * proper functions to avoid issues. + * @return The current trade factory in use if any. + */ function tradeFactory() public view returns (address) { return _tradeFactory; } + /** + * @notice Get the current tokens being sold through the Trade Factory. + * @dev We use a getter so the array can only be set through the + * proper functions to avoid issues. + * @return The current array of tokens being sold if any. + */ function rewardTokens() public view returns (address[] memory) { return _rewardTokens; } + /** + * @dev Add an array of tokens to sell to its corresponding `_to_. + */ function _addTokens(address[] memory _from, address[] memory _to) internal { for (uint256 i; i < _from.length; ++i) { _addToken(_from[i], _to[i]); } } + /** + * @dev Add the `_tokenFrom` to be sold to `_tokenTo` through the Trade Factory + */ function _addToken(address _tokenFrom, address _tokenTo) internal { address _tf = tradeFactory(); if (_tf != address(0)) { @@ -40,6 +65,10 @@ abstract contract TradeFactorySwapper { _rewardTokens.push(_tokenFrom); } + /** + * @dev Remove a specific `_tokenFrom` that was previously added to not be + * sold through the Trade Factory any more. + */ function _removeToken(address _tokenFrom, address _tokenTo) internal { address[] memory _rewardTokensLocal = rewardTokens(); address _tf = tradeFactory(); @@ -47,7 +76,9 @@ abstract contract TradeFactorySwapper { if (_rewardTokensLocal[i] == _tokenFrom) { if (i != _rewardTokensLocal.length - 1) { // if it isn't the last token, swap with the last one/ - _rewardTokensLocal[i] = _rewardTokensLocal[_rewardTokensLocal.length - 1]; + _rewardTokensLocal[i] = _rewardTokensLocal[ + _rewardTokensLocal.length - 1 + ]; } ERC20(_tokenFrom).safeApprove(_tf, 0); ITradeFactory(_tf).disable(_tokenFrom, _tokenTo); @@ -59,15 +90,24 @@ abstract contract TradeFactorySwapper { } } + /** + * @dev Removes all reward tokens and delete the Trade Factory. + */ function _deleteRewardTokens() internal { _removeTradeFactoryPermissions(); delete _rewardTokens; } + /** + * @dev Set a new instance of the Trade Factory. + * This will remove any old approvals for current factory if any. + * Then will add the new approvals for the new Trade Factory. + * Can pass in address(0) for `tradeFactory_` to remove all permissions. + */ function _setTradeFactory( address tradeFactory_, address _tokenTo - ) internal { + ) internal { address _tf = tradeFactory(); // Remove any old Trade Factory @@ -80,18 +120,20 @@ abstract contract TradeFactorySwapper { address[] memory _rewardTokensLocal = _rewardTokens; - // TODO: Dont iterate over the array twice for (uint256 i; i < _rewardTokensLocal.length; ++i) { address token = _rewardTokensLocal[i]; - ERC20(token).safeApprove(_tf, type(uint256).max); - - ITradeFactory(_tf).enable(token, _tokenTo); + ERC20(token).safeApprove(tradeFactory_, type(uint256).max); + ITradeFactory(tradeFactory_).enable(token, _tokenTo); } + // Set to storage _tradeFactory = tradeFactory_; } + /** + * @dev Remove any active approvals and set the trade factory to address(0). + */ function _removeTradeFactoryPermissions() internal { address[] memory rewardTokensLocal = rewardTokens(); address _tf = tradeFactory(); @@ -102,12 +144,16 @@ abstract contract TradeFactorySwapper { _tradeFactory = address(0); } - // Used for TradeFactory to claim rewards + /** + * @notice Used for TradeFactory to claim rewards. + */ function claimRewards() external { require(msg.sender == _tradeFactory, "!authorized"); _claimRewards(); } - // Need to be overridden to claim rewards mid report cycles. + /** + * @dev Need to be overridden to claim rewards mid report cycles. + */ function _claimRewards() internal virtual; } From 2c5efd6cfaa81586e08fd4ca22633f1c746b08b6 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 10:19:31 -0600 Subject: [PATCH 4/6] fix: order --- src/swappers/TradeFactorySwapper.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index b2085cc..055e51d 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -70,8 +70,8 @@ abstract contract TradeFactorySwapper { * sold through the Trade Factory any more. */ function _removeToken(address _tokenFrom, address _tokenTo) internal { - address[] memory _rewardTokensLocal = rewardTokens(); address _tf = tradeFactory(); + address[] memory _rewardTokensLocal = rewardTokens(); for (uint256 i; i < _rewardTokensLocal.length; ++i) { if (_rewardTokensLocal[i] == _tokenFrom) { if (i != _rewardTokensLocal.length - 1) { @@ -135,8 +135,8 @@ abstract contract TradeFactorySwapper { * @dev Remove any active approvals and set the trade factory to address(0). */ function _removeTradeFactoryPermissions() internal { - address[] memory rewardTokensLocal = rewardTokens(); address _tf = tradeFactory(); + address[] memory rewardTokensLocal = rewardTokens(); for (uint256 i; i < rewardTokensLocal.length; ++i) { ERC20(rewardTokensLocal[i]).safeApprove(_tf, 0); } From c80de927f5b174959b664d826bc4081b5d7c6c0e Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 10:35:32 -0600 Subject: [PATCH 5/6] fix: remove token --- src/swappers/TradeFactorySwapper.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index 055e51d..0907fdb 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -80,9 +80,12 @@ abstract contract TradeFactorySwapper { _rewardTokensLocal.length - 1 ]; } - ERC20(_tokenFrom).safeApprove(_tf, 0); - ITradeFactory(_tf).disable(_tokenFrom, _tokenTo); + if (_tf != address(0)) { + ERC20(_tokenFrom).safeApprove(_tf, 0); + ITradeFactory(_tf).disable(_tokenFrom, _tokenTo); + } + // Set to storage _rewardTokens = _rewardTokensLocal; _rewardTokens.pop(); From de1f6f47ef6efdf4714977d850e7bc333ba38327 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 28 Mar 2024 10:42:35 -0600 Subject: [PATCH 6/6] fix: lint --- src/swappers/TradeFactorySwapper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/swappers/TradeFactorySwapper.sol b/src/swappers/TradeFactorySwapper.sol index 0907fdb..cbd454f 100644 --- a/src/swappers/TradeFactorySwapper.sol +++ b/src/swappers/TradeFactorySwapper.sol @@ -85,7 +85,7 @@ abstract contract TradeFactorySwapper { ERC20(_tokenFrom).safeApprove(_tf, 0); ITradeFactory(_tf).disable(_tokenFrom, _tokenTo); } - + // Set to storage _rewardTokens = _rewardTokensLocal; _rewardTokens.pop();