diff --git a/CLAUDE.md b/CLAUDE.md index bd5815ab47..4f1d08b414 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,6 +44,7 @@ pnpm test # Mainnet unit tests pnpm test:base # Base network unit tests pnpm test:sonic # Sonic network unit tests pnpm test:coverage # Mainnet unit tests with coverage +pnpm test test/**/FILE_NAME.js # Running a specific test file ``` ### Fork Tests (require `PROVIDER_URL` in `.env`) diff --git a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol index 23713a8097..b23d607895 100644 --- a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol +++ b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol @@ -1,16 +1,11 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.0; -import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { AbstractSafeModule } from "./AbstractSafeModule.sol"; -import { ISafe } from "../interfaces/ISafe.sol"; import { IStrategy } from "../interfaces/IStrategy.sol"; -import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { - using SafeERC20 for IERC20; - mapping(address => bool) public isStrategyWhitelisted; address[] public strategies; @@ -39,22 +34,25 @@ contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { function claimRewards(bool silent) external onlyRole(OPERATOR_ROLE) { uint256 strategiesLength = strategies.length; for (uint256 i = 0; i < strategiesLength; i++) { - address strategy = strategies[i]; - - // Execute `collectRewardTokens` for all strategies - bool success = safeContract.execTransactionFromModule( - strategy, // To - 0, // Value - abi.encodeWithSelector(IStrategy.collectRewardTokens.selector), - 0 // Call - ); + _claimRewardsFor(strategies[i], silent); + } + } - if (!success) { - emit ClaimRewardsFailed(strategy); - } + /** + * @dev Internal: collect rewards from one strategy. + */ + function _claimRewardsFor(address strategy, bool silent) internal { + bool success = safeContract.execTransactionFromModule( + strategy, + 0, // Value + abi.encodeWithSelector(IStrategy.collectRewardTokens.selector), + 0 // Call + ); - require(success || silent, "Failed to claim rewards"); + if (!success) { + emit ClaimRewardsFailed(strategy); } + require(success || silent, "Failed to claim rewards"); } /** diff --git a/contracts/contracts/mocks/MockClaimableStrategy.sol b/contracts/contracts/mocks/MockClaimableStrategy.sol new file mode 100644 index 0000000000..86470e50fa --- /dev/null +++ b/contracts/contracts/mocks/MockClaimableStrategy.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.0; + +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +/** + * @title MockClaimableStrategy + * @dev Mock strategy for testing ClaimStrategyRewardsSafeModule. + * Holds reward tokens and transfers them to msg.sender on collectRewardTokens(). + */ +contract MockClaimableStrategy { + address[] private _rewardTokenAddresses; + bool public shouldRevert; + + function setRewardTokenAddresses(address[] memory tokens) external { + _rewardTokenAddresses = tokens; + } + + function setShouldRevert(bool _shouldRevert) external { + shouldRevert = _shouldRevert; + } + + function getRewardTokenAddresses() + external + view + returns (address[] memory) + { + return _rewardTokenAddresses; + } + + /** + * @dev Transfers all held reward tokens to msg.sender (the Safe). + * Reverts if shouldRevert is set, causing the Safe module exec to return false. + */ + function collectRewardTokens() external { + require(!shouldRevert, "MockClaimableStrategy: forced revert"); + for (uint256 i = 0; i < _rewardTokenAddresses.length; i++) { + uint256 balance = IERC20(_rewardTokenAddresses[i]).balanceOf( + address(this) + ); + if (balance > 0) { + IERC20(_rewardTokenAddresses[i]).transfer(msg.sender, balance); + } + } + } +} diff --git a/contracts/contracts/strategies/BaseCurveAMOStrategy.sol b/contracts/contracts/strategies/BaseCurveAMOStrategy.sol index 0b473c93db..718a6d865a 100644 --- a/contracts/contracts/strategies/BaseCurveAMOStrategy.sol +++ b/contracts/contracts/strategies/BaseCurveAMOStrategy.sol @@ -547,7 +547,7 @@ contract BaseCurveAMOStrategy is InitializableAbstractStrategy { function collectRewardTokens() external override - onlyHarvester + onlyHarvesterOrStrategist nonReentrant { // CRV rewards flow. diff --git a/contracts/contracts/strategies/CurveAMOStrategy.sol b/contracts/contracts/strategies/CurveAMOStrategy.sol index f00ddb49d6..ee18a5a4e8 100644 --- a/contracts/contracts/strategies/CurveAMOStrategy.sol +++ b/contracts/contracts/strategies/CurveAMOStrategy.sol @@ -586,7 +586,7 @@ contract CurveAMOStrategy is InitializableAbstractStrategy { function collectRewardTokens() external override - onlyHarvester + onlyHarvesterOrStrategist nonReentrant { // Collect CRV rewards from inflation diff --git a/contracts/contracts/strategies/algebra/StableSwapAMMStrategy.sol b/contracts/contracts/strategies/algebra/StableSwapAMMStrategy.sol index 0c7a0d62c6..9caee8f427 100644 --- a/contracts/contracts/strategies/algebra/StableSwapAMMStrategy.sol +++ b/contracts/contracts/strategies/algebra/StableSwapAMMStrategy.sol @@ -561,7 +561,7 @@ contract StableSwapAMMStrategy is InitializableAbstractStrategy { function collectRewardTokens() external override - onlyHarvester + onlyHarvesterOrStrategist nonReentrant { // Collect SWPx rewards from the gauge diff --git a/contracts/contracts/strategies/crosschain/CrossChainMasterStrategy.sol b/contracts/contracts/strategies/crosschain/CrossChainMasterStrategy.sol index f886bd58ad..9cfb4e2812 100644 --- a/contracts/contracts/strategies/crosschain/CrossChainMasterStrategy.sol +++ b/contracts/contracts/strategies/crosschain/CrossChainMasterStrategy.sol @@ -182,7 +182,7 @@ contract CrossChainMasterStrategy is function collectRewardTokens() external override - onlyHarvester + onlyHarvesterOrStrategist nonReentrant {} diff --git a/contracts/contracts/utils/InitializableAbstractStrategy.sol b/contracts/contracts/utils/InitializableAbstractStrategy.sol index dbb4adb097..3873f72143 100644 --- a/contracts/contracts/utils/InitializableAbstractStrategy.sol +++ b/contracts/contracts/utils/InitializableAbstractStrategy.sol @@ -119,16 +119,28 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /** * @notice Collect accumulated reward token and send to Vault. + * No-ops when the harvester address is not set. */ - function collectRewardTokens() external virtual onlyHarvester nonReentrant { + function collectRewardTokens() + external + virtual + onlyHarvesterOrStrategist + nonReentrant + { + if (harvesterAddress == address(0)) { + return; + } _collectRewardTokens(); } /** - * @dev Default implementation that transfers reward tokens to the Harvester + * @dev Default implementation that transfers reward tokens to the Harvester. * Implementing strategies need to add custom logic to collect the rewards. */ function _collectRewardTokens() internal virtual { + if (harvesterAddress == address(0)) { + return; + } uint256 rewardTokenCount = rewardTokenAddresses.length; for (uint256 i = 0; i < rewardTokenCount; ++i) { IERC20 rewardToken = IERC20(rewardTokenAddresses[i]); @@ -153,10 +165,14 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { } /** - * @dev Verifies that the caller is the Harvester. + * @dev Verifies that the caller is the Harvester or Strategist. */ - modifier onlyHarvester() { - require(msg.sender == harvesterAddress, "Caller is not the Harvester"); + modifier onlyHarvesterOrStrategist() { + require( + msg.sender == harvesterAddress || + msg.sender == IVault(vaultAddress).strategistAddr(), + "Caller is not the Harvester or Strategist" + ); _; } @@ -295,7 +311,7 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { */ function setHarvesterAddress(address _harvesterAddress) external - onlyGovernor + onlyGovernorOrStrategist { emit HarvesterAddressesUpdated(harvesterAddress, _harvesterAddress); harvesterAddress = _harvesterAddress; diff --git a/contracts/deploy/deployActions.js b/contracts/deploy/deployActions.js index d314f1ac8b..c385cdbf33 100644 --- a/contracts/deploy/deployActions.js +++ b/contracts/deploy/deployActions.js @@ -1235,6 +1235,12 @@ const deploySafeModulesForUnitTests = async () => { mockAutoWithdrawalVault.address, addresses.dead, ]); + + await deployWithConfirmation("ClaimStrategyRewardsSafeModule", [ + cSafeContract.address, // safe + cSafeContract.address, // operator + [], // strategies — added per-test via addStrategy + ]); }; module.exports = { diff --git a/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js b/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js new file mode 100644 index 0000000000..b8102bba0c --- /dev/null +++ b/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js @@ -0,0 +1,129 @@ +const addresses = require("../../utils/addresses"); +const { deploymentWithGovernanceProposal } = require("../../utils/deploy"); + +module.exports = deploymentWithGovernanceProposal( + { + deployName: "187_upgrade_curve_amo_morpho_harvester", + forceDeploy: false, + reduceQueueTime: true, + deployerIsProposer: false, + proposalId: "", + }, + async ({ deployWithConfirmation }) => { + // ── Proxy handles ──────────────────────────────────────────────────────── + const cOUSDCurveAMOProxy = await ethers.getContract("OUSDCurveAMOProxy"); + const cOETHCurveAMOProxy = await ethers.getContract("OETHCurveAMOProxy"); + const cOUSDMorphoV2Proxy = await ethers.getContract( + "OUSDMorphoV2StrategyProxy" + ); + + // Strategy contract handles (used to call governor functions via proxy) + const cOUSDCurveAMO = await ethers.getContractAt( + "CurveAMOStrategy", + cOUSDCurveAMOProxy.address + ); + const cOETHCurveAMO = await ethers.getContractAt( + "CurveAMOStrategy", + cOETHCurveAMOProxy.address + ); + const cOUSDMorphoV2 = await ethers.getContractAt( + "MorphoV2Strategy", + cOUSDMorphoV2Proxy.address + ); + + // ── Deploy new implementations ──────────────────────────────────────────── + // Both AMOs use CurveAMOStrategy but with different constructor args, + // so they get distinct artifact names. + + // 1. OUSD/USDC Curve AMO + const dOUSDCurveAMO = await deployWithConfirmation( + "OUSDCurveAMOStrategy", + [ + [ + addresses.mainnet.curve.OUSD_USDC.pool, // platformAddress (Curve pool = LP token) + addresses.mainnet.VaultProxy, // vaultAddress + ], + addresses.mainnet.OUSDProxy, // _otoken + addresses.mainnet.USDC, // _hardAsset + addresses.mainnet.curve.OUSD_USDC.gauge, // _gauge + addresses.mainnet.CRVMinter, // _minter + ], + "CurveAMOStrategy", // actual contract to compile + true + ); + + // 2. OETH/WETH Curve AMO + const dOETHCurveAMO = await deployWithConfirmation( + "OETHCurveAMOStrategy", + [ + [ + addresses.mainnet.curve.OETH_WETH.pool, // platformAddress + addresses.mainnet.OETHVaultProxy, // vaultAddress + ], + addresses.mainnet.OETHProxy, // _otoken (OETH) + addresses.mainnet.WETH, // _hardAsset + addresses.mainnet.curve.OETH_WETH.gauge, // _gauge + addresses.mainnet.CRVMinter, // _minter + ], + "CurveAMOStrategy", + true + ); + + // 3. OUSD Morpho V2 + const dOUSDMorphoV2 = await deployWithConfirmation( + "MorphoV2Strategy", + [ + [ + addresses.mainnet.MorphoOUSDv2Vault, // platformAddress + addresses.mainnet.VaultProxy, // vaultAddress + ], + addresses.mainnet.USDC, // _assetToken + ], + undefined, + true + ); + + // ── Governance actions ──────────────────────────────────────────────────── + return { + name: "Upgrade Curve AMO and Morpho V2 strategy implementations and set CoW Harvester", + actions: [ + // 1. Upgrade OUSD Curve AMO + { + contract: cOUSDCurveAMOProxy, + signature: "upgradeTo(address)", + args: [dOUSDCurveAMO.address], + }, + // 2. Set Multichain Strategist on OUSD Curve AMO + { + contract: cOUSDCurveAMO, + signature: "setHarvesterAddress(address)", + args: [addresses.multichainStrategist], + }, + // 3. Upgrade OETH Curve AMO + { + contract: cOETHCurveAMOProxy, + signature: "upgradeTo(address)", + args: [dOETHCurveAMO.address], + }, + // 4. Set Multichain Strategist on OETH Curve AMO + { + contract: cOETHCurveAMO, + signature: "setHarvesterAddress(address)", + args: [addresses.multichainStrategist], + }, + // 5. Upgrade OUSD Morpho V2 + { + contract: cOUSDMorphoV2Proxy, + signature: "upgradeTo(address)", + args: [dOUSDMorphoV2.address], + }, + // 6. Set CoW Harvester on OUSD Morpho V2 + { + contract: cOUSDMorphoV2, + signature: "setHarvesterAddress(address)", + args: [addresses.mainnet.CoWHarvester], + }, + ], + }; + } +); diff --git a/contracts/test/behaviour/harvestable.js b/contracts/test/behaviour/harvestable.js index 714f611961..6d810a9ba5 100644 --- a/contracts/test/behaviour/harvestable.js +++ b/contracts/test/behaviour/harvestable.js @@ -7,10 +7,12 @@ const { impersonateAndFund } = require("../../utils/signers"); * @param {*} context a function that returns a fixture with the additional properties: * - harvester: the OUSD or OETH harvester contract. * - strategy: the strategy to test + * - newBehavior: (optional, default false) set true for strategies upgraded with + * the new onlyHarvester modifier that also accepts the strategist. * @example shouldBehaveLikeHarvester(() => ({ ...fixture, - harvester: fixture.oethHarvester + harvester: fixture.oethHarvester, strategy: fixture.nativeStakingSSVStrategy, })); */ @@ -22,13 +24,24 @@ const shouldBehaveLikeHarvestable = (context) => { const harvesterSigner = await impersonateAndFund(harvester.address); await strategy.connect(harvesterSigner).collectRewardTokens(); }); + + it("Should allow strategist to collect rewards", async () => { + const { newBehavior, strategist, strategy } = context(); + if (!newBehavior) return; + await strategy.connect(strategist).collectRewardTokens(); + }); + it("Should NOT allow rewards to be collected by non-harvester", async () => { - const { anna, governor, strategist, strategy } = context(); + const { newBehavior, anna, governor, strategy } = context(); + + const errMsg = newBehavior + ? "Caller is not the Harvester or Strategist" + : "Caller is not the Harvester"; - for (const signer of [anna, governor, strategist]) { + for (const signer of [anna, governor]) { await expect( strategy.connect(signer).collectRewardTokens() - ).to.be.revertedWith("Caller is not the Harvester"); + ).to.be.revertedWith(errMsg); } }); diff --git a/contracts/test/behaviour/strategy.js b/contracts/test/behaviour/strategy.js index 23880954cb..1bf5a24092 100644 --- a/contracts/test/behaviour/strategy.js +++ b/contracts/test/behaviour/strategy.js @@ -403,16 +403,31 @@ const shouldBehaveLikeStrategy = (context) => { expect(await strategy.harvesterAddress()).to.equal(randomAddress); }); it("Should not allow the harvester to be set by non-governor", async () => { - const { strategy, strategist, matt, harvester, vault } = context(); + const { newBehavior, strategy, strategist, matt, harvester, vault } = + context(); const randomAddress = Wallet.createRandom().address; const vaultSigner = await impersonateAndFund(vault.address); - const harvesterSigner = await impersonateAndFund(harvester.address); - for (const signer of [strategist, matt, harvesterSigner, vaultSigner]) { - await expect( - strategy.connect(signer).setHarvesterAddress(randomAddress) - ).to.revertedWith("Caller is not the Governor"); + if (newBehavior) { + // Upgraded contracts: strategist CAN set harvester — use a random address + // that is definitely not the governor or strategist. + const randomSigner = await impersonateAndFund( + Wallet.createRandom().address + ); + for (const signer of [matt, randomSigner, vaultSigner]) { + await expect( + strategy.connect(signer).setHarvesterAddress(randomAddress) + ).to.revertedWith("Caller is not the Strategist or Governor"); + } + } else { + // Old contracts: only governor can set harvester. + const harvesterSigner = await impersonateAndFund(harvester.address); + for (const signer of [strategist, matt, harvesterSigner, vaultSigner]) { + await expect( + strategy.connect(signer).setHarvesterAddress(randomAddress) + ).to.revertedWith("Caller is not the Governor"); + } } }); it("Should allow reward tokens to be set by the governor", async () => { diff --git a/contracts/test/strategies/compoundingSSVStaking.js b/contracts/test/strategies/compoundingSSVStaking.js index 455db0f4fe..4b1ae961ac 100644 --- a/contracts/test/strategies/compoundingSSVStaking.js +++ b/contracts/test/strategies/compoundingSSVStaking.js @@ -123,6 +123,7 @@ describe("Unit test: Compounding SSV Staking Strategy", function () { valueAssets: [], harvester: fixture.oethHarvester, vault: fixture.oethVault, + newBehavior: true, })); describe("Initial setup", () => { diff --git a/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js b/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js index 09b428b5de..c91a764d80 100644 --- a/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js @@ -952,6 +952,7 @@ describe("Curve AMO OETH strategy", function () { governor: governor, strategist: rafael, harvester: harvester, + newBehavior: true, beforeEach: async () => { await balancePool(); @@ -971,7 +972,8 @@ describe("Curve AMO OETH strategy", function () { governor: governor, oeth: oeth, harvester: harvester, - strategist: rafael, + strategist: impersonatedStrategist, + newBehavior: true, })); const mintAndDepositToStrategy = async ({ diff --git a/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js b/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js index 2d89d6bf54..63d4f786db 100644 --- a/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js @@ -956,6 +956,7 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { governor: governor, strategist: rafael, harvester: harvester, + newBehavior: true, beforeEach: async () => { await balancePool(); @@ -975,7 +976,8 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { governor: governor, oeth: ousd, harvester: harvester, - strategist: rafael, + strategist: impersonatedStrategist, + newBehavior: true, })); const mintAndDepositToStrategy = async ({ diff --git a/contracts/test/strategies/nativeSSVStaking.js b/contracts/test/strategies/nativeSSVStaking.js index 6ba1316e04..b4493c92be 100644 --- a/contracts/test/strategies/nativeSSVStaking.js +++ b/contracts/test/strategies/nativeSSVStaking.js @@ -74,6 +74,7 @@ describe("Unit test: Native SSV Staking Strategy", function () { ...fixture, harvester: fixture.oethHarvester, strategy: fixture.nativeStakingSSVStrategy, + newBehavior: true, })); shouldBehaveLikeStrategy(() => ({ @@ -83,6 +84,7 @@ describe("Unit test: Native SSV Staking Strategy", function () { valueAssets: [], harvester: fixture.oethHarvester, vault: fixture.oethVault, + newBehavior: true, })); describe("Initial setup", function () { diff --git a/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js b/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js index 22279dc9a3..53a2e831cd 100644 --- a/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js +++ b/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js @@ -47,7 +47,7 @@ describe("ForkTest: Yearn's Morpho OUSD v2 Strategy", function () { addresses.mainnet.Timelock ); expect(await morphoOUSDv2Strategy.harvesterAddress()).to.equal( - addresses.multichainStrategist + addresses.mainnet.CoWHarvester ); }); it("Should be able to check balance", async () => { diff --git a/contracts/utils/addresses.js b/contracts/utils/addresses.js index 3051024a40..cf086619a6 100644 --- a/contracts/utils/addresses.js +++ b/contracts/utils/addresses.js @@ -388,6 +388,8 @@ addresses.mainnet.CampaignCreator = addresses.mainnet.MorphoOethUsdcMarket = "0xb8fef900b383db2dbbf4458c7f46acf5b140f26d603a6d1829963f241b82510e"; +addresses.mainnet.CoWHarvester = "0xD400341aEfED0BC75176714cFdE82e8BDAA2D3b8"; + // Arbitrum One addresses.arbitrumOne = {}; addresses.arbitrumOne.WOETHProxy = "0xD8724322f44E5c58D7A815F542036fb17DbbF839";