From 1522ea08c096d41ae3bc3a44fc2e16fc0475a686 Mon Sep 17 00:00:00 2001 From: katzman Date: Mon, 2 Oct 2023 22:41:27 +0000 Subject: [PATCH 1/4] Refactored Flux to use abstract APStrategy_V1 instead of duplicating it and implementing the interface directly --- contracts/core/strategy/APStrategy_V1.sol | 35 ++++++--- contracts/integrations/flux/FluxStrategy.sol | 79 ++++---------------- 2 files changed, 39 insertions(+), 75 deletions(-) diff --git a/contracts/core/strategy/APStrategy_V1.sol b/contracts/core/strategy/APStrategy_V1.sol index 3b13ebf40..7e51e2ffe 100644 --- a/contracts/core/strategy/APStrategy_V1.sol +++ b/contracts/core/strategy/APStrategy_V1.sol @@ -6,22 +6,36 @@ import {IStrategy} from "./IStrategy.sol"; import {Pausable} from "@openzeppelin/contracts/security/Pausable.sol"; abstract contract APStrategy_V1 is IStrategy, Pausable { - /*////////////////////////////////////////////////////////////// - CONFIG - //////////////////////////////////////////////////////////////*/ + /*** CONSTNATS ***/ + uint256 constant EXP_SCALE = 1e18; + + /*** STORAGE ***/ StrategyConfig public config; - constructor(StrategyConfig memory _config) { - config = _config; - } + /*////////////////////////////////////////////////////////////// + CONFIG + //////////////////////////////////////////////////////////////*/ + /// @notice Returns the config struct + /// @return Config the current strategy config function getStrategyConfig() external view returns (StrategyConfig memory) { return config; } + /// @notice Set the strategy config + /// @dev This method must be access controlled. The config overwrites the stored config in its entirety + /// @param _config The StrategyConfig that will be stored and referenced within the strategy contract function setStrategyConfig(StrategyConfig memory _config) external onlyAdmin { config = _config; + emit ConfigChanged(config); + } + + /// @notice Check whether the contract is paused + /// @dev Make public the state of the Pausable contract's `paused` state + /// @return paused the current state of the paused boolean + function paused() public view override(IStrategy, Pausable) returns (bool) { + return super.paused(); } /*////////////////////////////////////////////////////////////// @@ -29,7 +43,7 @@ abstract contract APStrategy_V1 is IStrategy, Pausable { //////////////////////////////////////////////////////////////*/ modifier onlyAdmin() { - require(_msgSender() == config.admin); + if (_msgSender() != config.admin) revert AdminOnly(); _; } @@ -40,9 +54,10 @@ abstract contract APStrategy_V1 is IStrategy, Pausable { function unpause() external virtual onlyAdmin { _unpause(); } - - function paused() public view virtual override(IStrategy, Pausable) returns (bool) { - return super.paused(); + + modifier nonZeroAmount(uint256 amt) { + if (amt == 0) revert ZeroAmount(); + _; } /*////////////////////////////////////////////////////////////// diff --git a/contracts/integrations/flux/FluxStrategy.sol b/contracts/integrations/flux/FluxStrategy.sol index 5f007b394..0024cda60 100644 --- a/contracts/integrations/flux/FluxStrategy.sol +++ b/contracts/integrations/flux/FluxStrategy.sol @@ -2,71 +2,20 @@ // author: @stevieraykatz pragma solidity ^0.8.19; -import {IStrategy} from "../../core/strategy/IStrategy.sol"; -import {Pausable} from "@openzeppelin/contracts/security/Pausable.sol"; +import {APStrategy_V1} from "../../core/strategy/APStrategy_V1.sol"; import {IFlux} from "./IFlux.sol"; import {FixedPointMathLib} from "../../lib/FixedPointMathLib.sol"; import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -contract FluxStrategy is IStrategy, Pausable, ReentrancyGuard { +contract FluxStrategy is APStrategy_V1, ReentrancyGuard { using FixedPointMathLib for uint256; - /*** CONSTNATS ***/ - uint256 constant expScale = 1e18; - - /*** STORAGE ***/ - StrategyConfig public config; - constructor(StrategyConfig memory _config) { config = _config; } - /*////////////////////////////////////////////////////////////// - ADMIN - //////////////////////////////////////////////////////////////*/ - - modifier onlyAdmin() { - if (_msgSender() != config.admin) revert AdminOnly(); - _; - } - - modifier nonZeroAmount(uint256 amt) { - if (amt == 0) revert ZeroAmount(); - _; - } - - function pause() external onlyAdmin { - _pause(); - } - - function unpause() external onlyAdmin { - _unpause(); - } - - /// @notice Check whether the contract is paused - /// @dev Make public the state of the Pausable contract's `paused` state - /// @return paused the current state of the paused boolean - function paused() public view override(IStrategy, Pausable) returns (bool) { - return super.paused(); - } - - /*////////////////////////////////////////////////////////////// - CONFIG - //////////////////////////////////////////////////////////////*/ - /// @notice Returns the config struct - /// @return Config the current strategy config - function getStrategyConfig() external view returns (StrategyConfig memory) { - return config; - } - - /// @notice Set the strategy config - /// @dev This method must be access controlled. The config overwrites the stored config in its entirety - /// @param _newConfig The StrategyConfig that willbe stored and referenced within the strategy contract - function setStrategyConfig(StrategyConfig memory _newConfig) external onlyAdmin { - config = _newConfig; - emit ConfigChanged(config); - } - /*////////////////////////////////////////////////////////////// IMPLEMENTATION //////////////////////////////////////////////////////////////*/ @@ -79,7 +28,7 @@ contract FluxStrategy is IStrategy, Pausable, ReentrancyGuard { /// @return yieldTokenAmt the qty of `config.yieldToken` that were yielded from the deposit action function deposit( uint256 amt - ) external payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { + ) external override payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { if (!IFlux(config.baseToken).transferFrom(_msgSender(), address(this), amt)) { revert TransferFailed(); } @@ -103,7 +52,7 @@ contract FluxStrategy is IStrategy, Pausable, ReentrancyGuard { /// @return baseTokenAmt the qty of `config.baseToken` that are approved for transfer by msg.sender function withdraw( uint256 amt - ) external payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { + ) external override payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { if (!IFlux(config.yieldToken).transferFrom(_msgSender(), address(this), amt)) { revert TransferFailed(); } @@ -122,22 +71,22 @@ contract FluxStrategy is IStrategy, Pausable, ReentrancyGuard { /// @dev This method expects that the `amt` provided is denominated in `baseToken` /// @param amt the qty of the `baseToken` that should be checked for conversion rates /// @return yieldTokenAmt the expected qty of `yieldToken` if this strategy received `amt` of `baseToken` - function previewDeposit(uint256 amt) external view returns (uint256) { - // Exchange Rate == (expScale * USDC) / fUSDC + function previewDeposit(uint256 amt) external override view returns (uint256) { + // Exchange Rate == (EXP_SCALE * USDC) / fUSDC uint256 exRate = IFlux(config.yieldToken).exchangeRateStored(); - // Expected fUSDC == (amtUSDC * expScale / exRate) - return amt.mulDivDown(expScale, exRate); + // Expected fUSDC == (amtUSDC * EXP_SCALE / exRate) + return amt.mulDivDown(EXP_SCALE, exRate); } /// @notice Provide an estimate for the current exchange rate for a given withdrawal /// @dev This method expects that the `amt` provided is denominated in `yieldToken` /// @param amt the qty of the `yieldToken` that should be checked for conversion rates /// @return yieldTokenAmt the expected qty of `baseToken` if this strategy received `amt` of `yieldToken` - function previewWithdraw(uint256 amt) external view returns (uint256) { - // Exchange Rate == (expScale * USDC) / fUSDC + function previewWithdraw(uint256 amt) external override view returns (uint256) { + // Exchange Rate == (EXP_SCALE * USDC) / fUSDC uint256 exRate = IFlux(config.yieldToken).exchangeRateStored(); - // Expected USDC == (amtfUSDC * exRate) / expScale - return amt.mulDivDown(exRate, expScale); + // Expected USDC == (amtfUSDC * exRate) / EXP_SCALE + return amt.mulDivDown(exRate, EXP_SCALE); } /*////////////////////////////////////////////////////////////// From 8498cac2cfe4dd6fea97d9b32e724bff101db334 Mon Sep 17 00:00:00 2001 From: katzman Date: Mon, 2 Oct 2023 22:44:02 +0000 Subject: [PATCH 2/4] Switched to SafeERC20 --- contracts/integrations/flux/FluxStrategy.sol | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/contracts/integrations/flux/FluxStrategy.sol b/contracts/integrations/flux/FluxStrategy.sol index 0024cda60..b7ea25b1d 100644 --- a/contracts/integrations/flux/FluxStrategy.sol +++ b/contracts/integrations/flux/FluxStrategy.sol @@ -11,6 +11,7 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol contract FluxStrategy is APStrategy_V1, ReentrancyGuard { using FixedPointMathLib for uint256; + using SafeERC20 for IERC20; constructor(StrategyConfig memory _config) { config = _config; @@ -29,12 +30,8 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { function deposit( uint256 amt ) external override payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { - if (!IFlux(config.baseToken).transferFrom(_msgSender(), address(this), amt)) { - revert TransferFailed(); - } - if (!IFlux(config.baseToken).approve(config.yieldToken, amt)) { - revert ApproveFailed(); - } + IERC20(config.baseToken).safeTransferFrom(_msgSender(), address(this), amt); + IERC20(config.baseToken).safeApprove(config.yieldToken, amt); uint256 yieldTokens = _enterPosition(amt); if (!IFlux(config.yieldToken).approve(_msgSender(), yieldTokens)) { revert ApproveFailed(); @@ -60,9 +57,7 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { revert ApproveFailed(); } uint256 baseTokens = _withdrawPosition(amt); - if (!IFlux(config.baseToken).approve(_msgSender(), baseTokens)) { - revert ApproveFailed(); - } + IERC20(config.baseToken).safeApprove(_msgSender(), baseTokens); emit WithdrewPosition(amt, baseTokens); return baseTokens; } From 022433380f14936b0a80ebb664809a78a8e63b0d Mon Sep 17 00:00:00 2001 From: katzman Date: Mon, 2 Oct 2023 22:44:26 +0000 Subject: [PATCH 3/4] Lint --- contracts/core/strategy/APStrategy_V1.sol | 3 +-- contracts/integrations/flux/FluxStrategy.sol | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/core/strategy/APStrategy_V1.sol b/contracts/core/strategy/APStrategy_V1.sol index 7e51e2ffe..0a634bddd 100644 --- a/contracts/core/strategy/APStrategy_V1.sol +++ b/contracts/core/strategy/APStrategy_V1.sol @@ -6,7 +6,6 @@ import {IStrategy} from "./IStrategy.sol"; import {Pausable} from "@openzeppelin/contracts/security/Pausable.sol"; abstract contract APStrategy_V1 is IStrategy, Pausable { - /*** CONSTNATS ***/ uint256 constant EXP_SCALE = 1e18; @@ -54,7 +53,7 @@ abstract contract APStrategy_V1 is IStrategy, Pausable { function unpause() external virtual onlyAdmin { _unpause(); } - + modifier nonZeroAmount(uint256 amt) { if (amt == 0) revert ZeroAmount(); _; diff --git a/contracts/integrations/flux/FluxStrategy.sol b/contracts/integrations/flux/FluxStrategy.sol index b7ea25b1d..b2b1111b4 100644 --- a/contracts/integrations/flux/FluxStrategy.sol +++ b/contracts/integrations/flux/FluxStrategy.sol @@ -29,7 +29,7 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { /// @return yieldTokenAmt the qty of `config.yieldToken` that were yielded from the deposit action function deposit( uint256 amt - ) external override payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { + ) external payable override whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { IERC20(config.baseToken).safeTransferFrom(_msgSender(), address(this), amt); IERC20(config.baseToken).safeApprove(config.yieldToken, amt); uint256 yieldTokens = _enterPosition(amt); @@ -49,7 +49,7 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { /// @return baseTokenAmt the qty of `config.baseToken` that are approved for transfer by msg.sender function withdraw( uint256 amt - ) external override payable whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { + ) external payable override whenNotPaused nonReentrant nonZeroAmount(amt) returns (uint256) { if (!IFlux(config.yieldToken).transferFrom(_msgSender(), address(this), amt)) { revert TransferFailed(); } @@ -66,7 +66,7 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { /// @dev This method expects that the `amt` provided is denominated in `baseToken` /// @param amt the qty of the `baseToken` that should be checked for conversion rates /// @return yieldTokenAmt the expected qty of `yieldToken` if this strategy received `amt` of `baseToken` - function previewDeposit(uint256 amt) external override view returns (uint256) { + function previewDeposit(uint256 amt) external view override returns (uint256) { // Exchange Rate == (EXP_SCALE * USDC) / fUSDC uint256 exRate = IFlux(config.yieldToken).exchangeRateStored(); // Expected fUSDC == (amtUSDC * EXP_SCALE / exRate) @@ -77,7 +77,7 @@ contract FluxStrategy is APStrategy_V1, ReentrancyGuard { /// @dev This method expects that the `amt` provided is denominated in `yieldToken` /// @param amt the qty of the `yieldToken` that should be checked for conversion rates /// @return yieldTokenAmt the expected qty of `baseToken` if this strategy received `amt` of `yieldToken` - function previewWithdraw(uint256 amt) external override view returns (uint256) { + function previewWithdraw(uint256 amt) external view override returns (uint256) { // Exchange Rate == (EXP_SCALE * USDC) / fUSDC uint256 exRate = IFlux(config.yieldToken).exchangeRateStored(); // Expected USDC == (amtfUSDC * exRate) / EXP_SCALE From bd249411f076d5c4da7aa9dfbd1176b45bf85ef1 Mon Sep 17 00:00:00 2001 From: katzman Date: Mon, 2 Oct 2023 22:54:38 +0000 Subject: [PATCH 4/4] Fix test to expect SafeERC20 error --- test/integrations/flux/FluxStrategy.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/integrations/flux/FluxStrategy.ts b/test/integrations/flux/FluxStrategy.ts index 6d72a0750..c82862050 100644 --- a/test/integrations/flux/FluxStrategy.ts +++ b/test/integrations/flux/FluxStrategy.ts @@ -132,7 +132,9 @@ describe("FluxStrategy", function () { it("reverts if the baseToken transfer fails", async function () { await wait(baseToken.mint(await owner.getAddress(), 1)); await wait(baseToken.setTransferAllowed(false)); - await expect(flux.deposit(1)).to.be.revertedWithCustomError(flux, "TransferFailed"); + await expect(flux.deposit(1)).to.be.revertedWith( + "SafeERC20: ERC20 operation did not succeed" + ); await wait(baseToken.setTransferAllowed(true)); }); it("reverts if the baseToken approve fails", async function () { @@ -140,7 +142,9 @@ describe("FluxStrategy", function () { await wait(baseToken.approve(flux.address, 1)); await wait(baseToken.setApproveAllowed(false)); await wait(yieldToken.setResponseAmt(1)); - await expect(flux.deposit(1)).to.be.revertedWithCustomError(flux, "ApproveFailed"); + await expect(flux.deposit(1)).to.be.revertedWith( + "SafeERC20: ERC20 operation did not succeed" + ); await wait(baseToken.setApproveAllowed(true)); }); it("reverts if the deposit fails", async function () { @@ -216,7 +220,9 @@ describe("FluxStrategy", function () { await wait(yieldToken.approve(flux.address, 1)); await wait(yieldToken.setResponseAmt(1)); await wait(baseToken.setApproveAllowed(false)); - await expect(flux.withdraw(1)).to.be.revertedWithCustomError(flux, "ApproveFailed"); + await expect(flux.withdraw(1)).to.be.revertedWith( + "SafeERC20: ERC20 operation did not succeed" + ); await wait(baseToken.setApproveAllowed(true)); }); it("correctly executes the redemption", async function () {