From ccd4cfab6ffc367e362dd785284a1802d1f592c4 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Mon, 23 Mar 2026 22:41:57 +0400 Subject: [PATCH 1/9] Forward rewards to another address --- .../ClaimStrategyRewardsSafeModule.sol | 119 +++++- .../contracts/mocks/MockClaimableStrategy.sol | 46 +++ contracts/deploy/deployActions.js | 8 + .../187_claim_strategy_rewards_module_v2.js | 66 +++ contracts/test/_fixture.js | 34 ++ .../safe-modules/claim-strategy-rewards.js | 381 ++++++++++++++++++ contracts/utils/addresses.js | 2 + 7 files changed, 642 insertions(+), 14 deletions(-) create mode 100644 contracts/contracts/mocks/MockClaimableStrategy.sol create mode 100644 contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js create mode 100644 contracts/test/safe-modules/claim-strategy-rewards.js diff --git a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol index 23713a8097..c91a073c3a 100644 --- a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol +++ b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol @@ -1,7 +1,6 @@ // 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"; @@ -9,22 +8,32 @@ 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; + /// @notice Address to forward claimed reward tokens to + address public rewardsTo; + event StrategyAdded(address strategy); event StrategyRemoved(address strategy); + event RewardsToUpdated(address newRewardsTo); event ClaimRewardsFailed(address strategy); + event RewardTokensForwarded( + address strategy, + address token, + uint256 amount + ); + event ForwardRewardsFailed(address strategy, address token); constructor( address _safeAddress, address operator, - address[] memory _strategies + address[] memory _strategies, + address _rewardsTo ) AbstractSafeModule(_safeAddress) { _grantRole(OPERATOR_ROLE, operator); + _setRewardsTo(_rewardsTo); // Whitelist all strategies for (uint256 i = 0; i < _strategies.length; i++) { @@ -33,30 +42,112 @@ contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { } /** - * @dev Claim rewards from all whitelisted strategies + * @dev Claim rewards from all whitelisted strategies and forward to rewardsTo * @param silent Doesn't revert on error if set to true */ function claimRewards(bool silent) external onlyRole(OPERATOR_ROLE) { uint256 strategiesLength = strategies.length; for (uint256 i = 0; i < strategiesLength; i++) { - address strategy = strategies[i]; + _claimRewardsFor(strategies[i], silent); + } + } + + /** + * @dev Claim rewards from a single whitelisted strategy and forward to rewardsTo + * @param strategy The strategy to claim rewards from + * @param silent Doesn't revert on error if set to true + */ + function claimRewardsFor(address strategy, bool silent) + external + onlyRole(OPERATOR_ROLE) + { + require(isStrategyWhitelisted[strategy], "Strategy not whitelisted"); + _claimRewardsFor(strategy, silent); + } + + /** + * @dev Internal: collect rewards from one strategy and forward tokens to rewardsTo. + * Tokens land in the Safe after collectRewardTokens(), so forwarding also + * goes through execTransactionFromModule. + */ + function _claimRewardsFor(address strategy, bool silent) internal { + address[] memory rewardTokens = IStrategy(strategy) + .getRewardTokenAddresses(); + + // Snapshot Safe balances before claiming + uint256[] memory balancesBefore = new uint256[](rewardTokens.length); + for (uint256 j = 0; j < rewardTokens.length; j++) { + balancesBefore[j] = IERC20(rewardTokens[j]).balanceOf( + address(safeContract) + ); + } + + // Collect reward tokens into the Safe + bool success = safeContract.execTransactionFromModule( + strategy, + 0, // Value + abi.encodeWithSelector(IStrategy.collectRewardTokens.selector), + 0 // Call + ); - // Execute `collectRewardTokens` for all strategies - bool success = safeContract.execTransactionFromModule( - strategy, // To + if (!success) { + emit ClaimRewardsFailed(strategy); + } + require(success || silent, "Failed to claim rewards"); + + if (!success) { + return; + } + + // Forward newly collected tokens from the Safe to rewardsTo + for (uint256 j = 0; j < rewardTokens.length; j++) { + address token = rewardTokens[j]; + uint256 amount = IERC20(token).balanceOf(address(safeContract)) - + balancesBefore[j]; + if (amount == 0) { + continue; + } + + bool transferSuccess = safeContract.execTransactionFromModule( + token, 0, // Value - abi.encodeWithSelector(IStrategy.collectRewardTokens.selector), + abi.encodeWithSelector( + IERC20.transfer.selector, + rewardsTo, + amount + ), 0 // Call ); - if (!success) { - emit ClaimRewardsFailed(strategy); + if (!transferSuccess) { + emit ForwardRewardsFailed(strategy, token); + } else { + emit RewardTokensForwarded(strategy, token, amount); } - - require(success || silent, "Failed to claim rewards"); + require( + transferSuccess || silent, + "Failed to forward reward tokens" + ); } } + /** + * @dev Update the address that claimed reward tokens are forwarded to + * @param _rewardsTo New rewards destination address + */ + function setRewardsTo(address _rewardsTo) + external + onlyRole(DEFAULT_ADMIN_ROLE) + { + _setRewardsTo(_rewardsTo); + } + + function _setRewardsTo(address _rewardsTo) internal { + require(_rewardsTo != address(0), "Invalid rewardsTo address"); + rewardsTo = _rewardsTo; + emit RewardsToUpdated(_rewardsTo); + } + /** * @dev Add a strategy to the whitelist * @param _strategy The address of the strategy to add 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/deploy/deployActions.js b/contracts/deploy/deployActions.js index d314f1ac8b..16496caf1f 100644 --- a/contracts/deploy/deployActions.js +++ b/contracts/deploy/deployActions.js @@ -1235,6 +1235,14 @@ const deploySafeModulesForUnitTests = async () => { mockAutoWithdrawalVault.address, addresses.dead, ]); + + await deployWithConfirmation("MockClaimableStrategy"); + await deployWithConfirmation("ClaimStrategyRewardsSafeModule", [ + cSafeContract.address, // safe + cSafeContract.address, // operator + [], // strategies — added per-test via addStrategy + addresses.dead, // rewardsTo placeholder — overridden in tests via setRewardsTo + ]); }; module.exports = { diff --git a/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js b/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js new file mode 100644 index 0000000000..2945c59c62 --- /dev/null +++ b/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js @@ -0,0 +1,66 @@ +const addresses = require("../../utils/addresses"); +const { deploymentWithGovernanceProposal } = require("../../utils/deploy"); +const { impersonateAndFund } = require("../../utils/signers"); +const { isFork } = require("../../utils/hardhat-helpers"); + +module.exports = deploymentWithGovernanceProposal( + { + deployName: "187_claim_strategy_rewards_module_v2", + forceDeploy: false, + reduceQueueTime: true, + deployerIsProposer: false, + proposalId: "", + }, + async ({ deployWithConfirmation, withConfirmation }) => { + const safeAddress = addresses.multichainStrategist; + + // Strategies whose reward tokens will be claimed and forwarded. + const cOUSDCurveAMOProxy = await ethers.getContract("OUSDCurveAMOProxy"); + const cOETHCurveAMOProxy = await ethers.getContract("OETHCurveAMOProxy"); + const cOUSDMorphoV2StrategyProxy = await ethers.getContract( + "OUSDMorphoV2StrategyProxy" + ); + + const strategyAddresses = [ + cOUSDCurveAMOProxy.address, + cOETHCurveAMOProxy.address, + cOUSDMorphoV2StrategyProxy.address, + ]; + + // Address that claimed reward tokens are forwarded to. + // Defaults to the multichainStrategist Safe — confirm before deploying. + const rewardsTo = addresses.mainnet.CoWHarvester; + + await deployWithConfirmation("ClaimStrategyRewardsSafeModule", [ + safeAddress, // safe + addresses.mainnet.validatorRegistrator, // operator (Defender relayer) + strategyAddresses, + rewardsTo, + ]); + const cModule = await ethers.getContract("ClaimStrategyRewardsSafeModule"); + + console.log( + `ClaimStrategyRewardsSafeModule (for ${safeAddress}) deployed to`, + cModule.address + ); + console.log(`rewardsTo set to ${rewardsTo}`); + + if (isFork) { + const safeSigner = await impersonateAndFund(safeAddress); + const cSafe = await ethers.getContractAt( + ["function enableModule(address module) external"], + safeAddress + ); + + await withConfirmation( + cSafe.connect(safeSigner).enableModule(cModule.address) + ); + + console.log("Enabled module on fork"); + } + + return { + actions: [], + }; + } +); diff --git a/contracts/test/_fixture.js b/contracts/test/_fixture.js index df49dabcd4..9bb05c8330 100644 --- a/contracts/test/_fixture.js +++ b/contracts/test/_fixture.js @@ -930,6 +930,39 @@ async function claimRewardsModuleFixture() { }; } +async function claimStrategyRewardsModuleFixture() { + const fixture = await defaultFixture(); + + const claimRewardsModule = await ethers.getContract( + "ClaimStrategyRewardsSafeModule" + ); + const mockSafe = await ethers.getContract("MockSafeContract"); + const mockClaimableStrategy = await ethers.getContract( + "MockClaimableStrategy" + ); + const mockUSDC = await ethers.getContract("MockUSDC"); + const mockDAI = await ethers.getContract("MockDAI"); + + // MockSafeContract is both safe and operator in the unit-test deployment. + const safeSigner = await impersonateAndFund(mockSafe.address); + + // A stranger with no roles + const stranger = await impersonateAndFund( + "0x0000000000000000000000000000000000000002" + ); + + return { + ...fixture, + claimRewardsModule, + mockSafe, + mockClaimableStrategy, + mockUSDC, + mockDAI, + safeSigner, + stranger, + }; +} + async function autoWithdrawalModuleFixture() { const fixture = await defaultFixture(); @@ -1839,6 +1872,7 @@ module.exports = { bridgeHelperModuleFixture, beaconChainFixture, claimRewardsModuleFixture, + claimStrategyRewardsModuleFixture, autoWithdrawalModuleFixture, crossChainFixtureUnit, crossChainFixture, diff --git a/contracts/test/safe-modules/claim-strategy-rewards.js b/contracts/test/safe-modules/claim-strategy-rewards.js new file mode 100644 index 0000000000..dc80db7e21 --- /dev/null +++ b/contracts/test/safe-modules/claim-strategy-rewards.js @@ -0,0 +1,381 @@ +const { expect } = require("chai"); +const { + createFixtureLoader, + claimStrategyRewardsModuleFixture, +} = require("../_fixture"); +const addresses = require("../../utils/addresses"); +const { parseUnits } = require("ethers/lib/utils"); + +const fixture = createFixtureLoader(claimStrategyRewardsModuleFixture); + +describe("Unit Test: Claim Strategy Rewards Safe Module", function () { + let f; + let rewardsTo; + + beforeEach(async () => { + f = await fixture(); + // Use a deterministic test address as the rewards destination + rewardsTo = "0x0000000000000000000000000000000000000099"; + + // Set rewardsTo via the safe (admin) + await f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(rewardsTo); + }); + + // ─── Deployment ─────────────────────────────────────────────────────────── + + describe("Deployment", () => { + it("Should set safeContract", async () => { + expect(await f.claimRewardsModule.safeContract()).to.eq( + f.mockSafe.address + ); + }); + + it("Should set rewardsTo to addresses.dead initially", async () => { + // The freshly deployed contract has addresses.dead as rewardsTo + // (set in deploySafeModulesForUnitTests), but beforeEach overrides it. + // Verify the override took effect. + expect(await f.claimRewardsModule.rewardsTo()).to.eq(rewardsTo); + }); + }); + + // ─── setRewardsTo ───────────────────────────────────────────────────────── + + describe("setRewardsTo()", () => { + it("Should update rewardsTo and emit event", async () => { + const newAddress = "0x0000000000000000000000000000000000000042"; + await expect( + f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(newAddress) + ) + .to.emit(f.claimRewardsModule, "RewardsToUpdated") + .withArgs(newAddress); + + expect(await f.claimRewardsModule.rewardsTo()).to.eq(newAddress); + }); + + it("Should revert on zero address", async () => { + await expect( + f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(addresses.zero) + ).to.be.revertedWith("Invalid rewardsTo address"); + }); + + it("Should revert if called by non-admin", async () => { + await expect( + f.claimRewardsModule.connect(f.stranger).setRewardsTo(rewardsTo) + ).to.be.reverted; + }); + }); + + // ─── addStrategy / removeStrategy ───────────────────────────────────────── + + describe("addStrategy() / removeStrategy()", () => { + it("Should add and whitelist a strategy", async () => { + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + expect( + await f.claimRewardsModule.isStrategyWhitelisted( + f.mockClaimableStrategy.address + ) + ).to.be.true; + }); + + it("Should revert when adding a duplicate strategy", async () => { + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + await expect( + f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address) + ).to.be.revertedWith("Strategy already whitelisted"); + }); + + it("Should remove a strategy", async () => { + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + await f.claimRewardsModule + .connect(f.safeSigner) + .removeStrategy(f.mockClaimableStrategy.address); + + expect( + await f.claimRewardsModule.isStrategyWhitelisted( + f.mockClaimableStrategy.address + ) + ).to.be.false; + }); + + it("Should revert when removing a non-whitelisted strategy", async () => { + await expect( + f.claimRewardsModule + .connect(f.safeSigner) + .removeStrategy(f.mockClaimableStrategy.address) + ).to.be.revertedWith("Strategy not whitelisted"); + }); + + it("Should revert if called by non-admin", async () => { + await expect( + f.claimRewardsModule + .connect(f.stranger) + .addStrategy(f.mockClaimableStrategy.address) + ).to.be.reverted; + }); + }); + + // ─── claimRewardsFor ────────────────────────────────────────────────────── + + describe("claimRewardsFor()", () => { + it("Should revert for non-whitelisted strategy", async () => { + await expect( + f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false) + ).to.be.revertedWith("Strategy not whitelisted"); + }); + + it("Should revert if called by non-operator", async () => { + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + await expect( + f.claimRewardsModule + .connect(f.stranger) + .claimRewardsFor(f.mockClaimableStrategy.address, false) + ).to.be.reverted; + }); + + it("Should claim and forward tokens to rewardsTo", async () => { + const { mockUSDC } = f; + const amount = parseUnits("100", 6); + + // Configure the strategy with a reward token and fund it + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + await mockUSDC.mintTo(f.mockClaimableStrategy.address, amount); + + // Whitelist the strategy + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + const rewardsToBalanceBefore = await mockUSDC.balanceOf(rewardsTo); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false); + + await expect(tx) + .to.emit(f.claimRewardsModule, "RewardTokensForwarded") + .withArgs(f.mockClaimableStrategy.address, mockUSDC.address, amount); + + // Tokens should arrive at rewardsTo, not remain in the Safe + expect(await mockUSDC.balanceOf(rewardsTo)).to.eq( + rewardsToBalanceBefore.add(amount) + ); + expect(await mockUSDC.balanceOf(f.mockSafe.address)).to.eq(0); + }); + + it("Should do nothing when strategy has no reward tokens", async () => { + await f.mockClaimableStrategy.setRewardTokenAddresses([]); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false); + + await expect(tx).to.not.emit( + f.claimRewardsModule, + "RewardTokensForwarded" + ); + }); + + it("Should do nothing when reward token balance is zero", async () => { + const { mockUSDC } = f; + + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + // No tokens minted to strategy + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false); + + await expect(tx).to.not.emit( + f.claimRewardsModule, + "RewardTokensForwarded" + ); + }); + + it("Should emit ClaimRewardsFailed and revert when silent=false", async () => { + const { mockUSDC } = f; + + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + await f.mockClaimableStrategy.setShouldRevert(true); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + await expect( + f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false) + ).to.be.revertedWith("Failed to claim rewards"); + }); + + it("Should emit ClaimRewardsFailed but not revert when silent=true", async () => { + const { mockUSDC } = f; + + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + await f.mockClaimableStrategy.setShouldRevert(true); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, true); + + await expect(tx) + .to.emit(f.claimRewardsModule, "ClaimRewardsFailed") + .withArgs(f.mockClaimableStrategy.address); + }); + + it("Should handle multiple reward tokens", async () => { + const { mockUSDC, mockDAI } = f; + const usdcAmount = parseUnits("50", 6); + const daiAmount = parseUnits("200", 18); + + await f.mockClaimableStrategy.setRewardTokenAddresses([ + mockUSDC.address, + mockDAI.address, + ]); + await mockUSDC.mintTo(f.mockClaimableStrategy.address, usdcAmount); + await mockDAI.mintTo(f.mockClaimableStrategy.address, daiAmount); + + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewardsFor(f.mockClaimableStrategy.address, false); + + await expect(tx) + .to.emit(f.claimRewardsModule, "RewardTokensForwarded") + .withArgs( + f.mockClaimableStrategy.address, + mockUSDC.address, + usdcAmount + ); + await expect(tx) + .to.emit(f.claimRewardsModule, "RewardTokensForwarded") + .withArgs(f.mockClaimableStrategy.address, mockDAI.address, daiAmount); + + expect(await mockUSDC.balanceOf(rewardsTo)).to.eq(usdcAmount); + expect(await mockDAI.balanceOf(rewardsTo)).to.eq(daiAmount); + }); + }); + + // ─── claimRewards (claimAll) ─────────────────────────────────────────────── + + describe("claimRewards() — claim all", () => { + it("Should revert if called by non-operator", async () => { + await expect(f.claimRewardsModule.connect(f.stranger).claimRewards(false)) + .to.be.reverted; + }); + + it("Should claim and forward from all whitelisted strategies", async () => { + const { mockUSDC, mockDAI } = f; + + // Deploy a second mock claimable strategy by repurposing the fixture one + // and using a second ethers.getContract with a different address. + // For simplicity, add the same strategy twice would fail, so we use + // two separate strategies by deploying a second one inline. + const MockClaimableStrategy = await ethers.getContractFactory( + "MockClaimableStrategy" + ); + const strategy2 = await MockClaimableStrategy.deploy(); + + const amount1 = parseUnits("100", 6); + const amount2 = parseUnits("300", 18); + + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + await mockUSDC.mintTo(f.mockClaimableStrategy.address, amount1); + + await strategy2.setRewardTokenAddresses([mockDAI.address]); + await mockDAI.mintTo(strategy2.address, amount2); + + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(strategy2.address); + + await f.claimRewardsModule.connect(f.safeSigner).claimRewards(false); + + expect(await mockUSDC.balanceOf(rewardsTo)).to.eq(amount1); + expect(await mockDAI.balanceOf(rewardsTo)).to.eq(amount2); + expect(await mockUSDC.balanceOf(f.mockSafe.address)).to.eq(0); + expect(await mockDAI.balanceOf(f.mockSafe.address)).to.eq(0); + }); + + it("Should continue past failures when silent=true", async () => { + const { mockUSDC, mockDAI } = f; + + const MockClaimableStrategy = await ethers.getContractFactory( + "MockClaimableStrategy" + ); + const strategy2 = await MockClaimableStrategy.deploy(); + + const amount2 = parseUnits("300", 18); + + // strategy1 will revert + await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); + await f.mockClaimableStrategy.setShouldRevert(true); + + // strategy2 succeeds + await strategy2.setRewardTokenAddresses([mockDAI.address]); + await mockDAI.mintTo(strategy2.address, amount2); + + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(strategy2.address); + + const tx = await f.claimRewardsModule + .connect(f.safeSigner) + .claimRewards(true); + + await expect(tx) + .to.emit(f.claimRewardsModule, "ClaimRewardsFailed") + .withArgs(f.mockClaimableStrategy.address); + await expect(tx) + .to.emit(f.claimRewardsModule, "RewardTokensForwarded") + .withArgs(strategy2.address, mockDAI.address, amount2); + + expect(await mockDAI.balanceOf(rewardsTo)).to.eq(amount2); + }); + + it("Should stop on first failure when silent=false", async () => { + await f.mockClaimableStrategy.setRewardTokenAddresses([]); + await f.mockClaimableStrategy.setShouldRevert(true); + await f.claimRewardsModule + .connect(f.safeSigner) + .addStrategy(f.mockClaimableStrategy.address); + + await expect( + f.claimRewardsModule.connect(f.safeSigner).claimRewards(false) + ).to.be.revertedWith("Failed to claim rewards"); + }); + }); +}); diff --git a/contracts/utils/addresses.js b/contracts/utils/addresses.js index 3051024a40..e10c714e99 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"; From fa76e078371be888f3fc7c9713952d7c2020d694 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Mon, 23 Mar 2026 22:42:08 +0400 Subject: [PATCH 2/9] prettify --- contracts/utils/addresses.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/addresses.js b/contracts/utils/addresses.js index e10c714e99..cf086619a6 100644 --- a/contracts/utils/addresses.js +++ b/contracts/utils/addresses.js @@ -388,7 +388,7 @@ addresses.mainnet.CampaignCreator = addresses.mainnet.MorphoOethUsdcMarket = "0xb8fef900b383db2dbbf4458c7f46acf5b140f26d603a6d1829963f241b82510e"; -addresses.mainnet.CoWHarvester = "0xD400341aEfED0BC75176714cFdE82e8BDAA2D3b8" +addresses.mainnet.CoWHarvester = "0xD400341aEfED0BC75176714cFdE82e8BDAA2D3b8"; // Arbitrum One addresses.arbitrumOne = {}; From aa73bb5daa200e07d1840b45a6e01e25d32a45eb Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Tue, 24 Mar 2026 14:06:03 +0400 Subject: [PATCH 3/9] Make strategist harvest rewards --- .../ClaimStrategyRewardsSafeModule.sol | 99 +---- contracts/contracts/interfaces/IStrategy.sol | 6 + .../utils/InitializableAbstractStrategy.sol | 51 ++- contracts/deploy/deployActions.js | 2 - .../187_claim_strategy_rewards_module_v2.js | 66 --- .../187_upgrade_curve_amo_morpho_harvester.js | 129 ++++++ contracts/test/_fixture.js | 34 -- contracts/test/behaviour/harvestable.js | 68 +++- .../safe-modules/claim-strategy-rewards.js | 381 ------------------ .../curve-amo-oeth.mainnet.fork-test.js | 67 ++- .../curve-amo-ousd.mainnet.fork-test.js | 67 ++- .../ousd-v2-morpho.mainnet.fork-test.js | 98 +++++ 12 files changed, 480 insertions(+), 588 deletions(-) delete mode 100644 contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js create mode 100644 contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js delete mode 100644 contracts/test/safe-modules/claim-strategy-rewards.js diff --git a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol index c91a073c3a..b23d607895 100644 --- a/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol +++ b/contracts/contracts/automation/ClaimStrategyRewardsSafeModule.sol @@ -3,37 +3,23 @@ pragma solidity ^0.8.0; 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 { mapping(address => bool) public isStrategyWhitelisted; address[] public strategies; - /// @notice Address to forward claimed reward tokens to - address public rewardsTo; - event StrategyAdded(address strategy); event StrategyRemoved(address strategy); - event RewardsToUpdated(address newRewardsTo); event ClaimRewardsFailed(address strategy); - event RewardTokensForwarded( - address strategy, - address token, - uint256 amount - ); - event ForwardRewardsFailed(address strategy, address token); constructor( address _safeAddress, address operator, - address[] memory _strategies, - address _rewardsTo + address[] memory _strategies ) AbstractSafeModule(_safeAddress) { _grantRole(OPERATOR_ROLE, operator); - _setRewardsTo(_rewardsTo); // Whitelist all strategies for (uint256 i = 0; i < _strategies.length; i++) { @@ -42,7 +28,7 @@ contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { } /** - * @dev Claim rewards from all whitelisted strategies and forward to rewardsTo + * @dev Claim rewards from all whitelisted strategies * @param silent Doesn't revert on error if set to true */ function claimRewards(bool silent) external onlyRole(OPERATOR_ROLE) { @@ -53,36 +39,9 @@ contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { } /** - * @dev Claim rewards from a single whitelisted strategy and forward to rewardsTo - * @param strategy The strategy to claim rewards from - * @param silent Doesn't revert on error if set to true - */ - function claimRewardsFor(address strategy, bool silent) - external - onlyRole(OPERATOR_ROLE) - { - require(isStrategyWhitelisted[strategy], "Strategy not whitelisted"); - _claimRewardsFor(strategy, silent); - } - - /** - * @dev Internal: collect rewards from one strategy and forward tokens to rewardsTo. - * Tokens land in the Safe after collectRewardTokens(), so forwarding also - * goes through execTransactionFromModule. + * @dev Internal: collect rewards from one strategy. */ function _claimRewardsFor(address strategy, bool silent) internal { - address[] memory rewardTokens = IStrategy(strategy) - .getRewardTokenAddresses(); - - // Snapshot Safe balances before claiming - uint256[] memory balancesBefore = new uint256[](rewardTokens.length); - for (uint256 j = 0; j < rewardTokens.length; j++) { - balancesBefore[j] = IERC20(rewardTokens[j]).balanceOf( - address(safeContract) - ); - } - - // Collect reward tokens into the Safe bool success = safeContract.execTransactionFromModule( strategy, 0, // Value @@ -94,58 +53,6 @@ contract ClaimStrategyRewardsSafeModule is AbstractSafeModule { emit ClaimRewardsFailed(strategy); } require(success || silent, "Failed to claim rewards"); - - if (!success) { - return; - } - - // Forward newly collected tokens from the Safe to rewardsTo - for (uint256 j = 0; j < rewardTokens.length; j++) { - address token = rewardTokens[j]; - uint256 amount = IERC20(token).balanceOf(address(safeContract)) - - balancesBefore[j]; - if (amount == 0) { - continue; - } - - bool transferSuccess = safeContract.execTransactionFromModule( - token, - 0, // Value - abi.encodeWithSelector( - IERC20.transfer.selector, - rewardsTo, - amount - ), - 0 // Call - ); - - if (!transferSuccess) { - emit ForwardRewardsFailed(strategy, token); - } else { - emit RewardTokensForwarded(strategy, token, amount); - } - require( - transferSuccess || silent, - "Failed to forward reward tokens" - ); - } - } - - /** - * @dev Update the address that claimed reward tokens are forwarded to - * @param _rewardsTo New rewards destination address - */ - function setRewardsTo(address _rewardsTo) - external - onlyRole(DEFAULT_ADMIN_ROLE) - { - _setRewardsTo(_rewardsTo); - } - - function _setRewardsTo(address _rewardsTo) internal { - require(_rewardsTo != address(0), "Invalid rewardsTo address"); - rewardsTo = _rewardsTo; - emit RewardsToUpdated(_rewardsTo); } /** diff --git a/contracts/contracts/interfaces/IStrategy.sol b/contracts/contracts/interfaces/IStrategy.sol index f02f0f7689..3ea9f546d1 100644 --- a/contracts/contracts/interfaces/IStrategy.sol +++ b/contracts/contracts/interfaces/IStrategy.sol @@ -57,6 +57,12 @@ interface IStrategy { function harvesterAddress() external view returns (address); + function harvesterPaused() external view returns (bool); + + function pauseHarvester() external; + + function unpauseHarvester() external; + function transferToken(address token, uint256 amount) external; function setRewardTokenAddresses(address[] calldata _rewardTokenAddresses) diff --git a/contracts/contracts/utils/InitializableAbstractStrategy.sol b/contracts/contracts/utils/InitializableAbstractStrategy.sol index dbb4adb097..a862e887b6 100644 --- a/contracts/contracts/utils/InitializableAbstractStrategy.sol +++ b/contracts/contracts/utils/InitializableAbstractStrategy.sol @@ -32,6 +32,8 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { address _oldHarvesterAddress, address _newHarvesterAddress ); + event HarvesterPaused(address pausedBy); + event HarvesterUnpaused(address unpausedBy); /// @notice Address of the underlying platform address public immutable platformAddress; @@ -66,12 +68,17 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /// @notice Address of the reward tokens. eg CRV, BAL, CVX, AURA address[] public rewardTokenAddresses; + /// @notice Whether the harvester is paused. When true, collectRewardTokens + /// will not transfer rewards to the harvester address. + bool public harvesterPaused; + /* Reserved for future expansion. Used to be 100 storage slots * and has decreased to accommodate: * - harvesterAddress * - rewardTokenAddresses + * - harvesterPaused */ - int256[98] private _reserved; + int256[97] private _reserved; struct BaseStrategyConfig { address platformAddress; // Address of the underlying platform @@ -119,16 +126,27 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /** * @notice Collect accumulated reward token and send to Vault. + * No-ops when the harvester is not set or is paused — rewards + * remain in the strategy until the harvester is unpaused or reset. */ function collectRewardTokens() external virtual onlyHarvester nonReentrant { + if (harvesterAddress == address(0) || harvesterPaused) { + 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 { + // The harvester check is intentionally repeated here (it also exists in the + // external collectRewardTokens) because strategies may override the external + // function without calling super, bypassing that check. + if (harvesterAddress == address(0) || harvesterPaused) { + return; + } uint256 rewardTokenCount = rewardTokenAddresses.length; for (uint256 i = 0; i < rewardTokenCount; ++i) { IERC20 rewardToken = IERC20(rewardTokenAddresses[i]); @@ -153,10 +171,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"); + require( + msg.sender == harvesterAddress || + msg.sender == IVault(vaultAddress).strategistAddr(), + "Caller is not the Harvester or Strategist" + ); _; } @@ -301,6 +323,27 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { harvesterAddress = _harvesterAddress; } + /** + * @notice Pause reward token transfers to the Harvester. + * Callable by the Strategist for a fast response to a rogue harvester. + * While paused, collectRewardTokens() still runs but rewards remain + * in the strategy contract. + */ + function pauseHarvester() external onlyGovernorOrStrategist { + harvesterPaused = true; + emit HarvesterPaused(msg.sender); + } + + /** + * @notice Unpause reward token transfers to the Harvester. + * Governor only — intended to be called after setting a new + * harvester address via setHarvesterAddress(). + */ + function unpauseHarvester() external onlyGovernor { + harvesterPaused = false; + emit HarvesterUnpaused(msg.sender); + } + /*************************************** Abstract ****************************************/ diff --git a/contracts/deploy/deployActions.js b/contracts/deploy/deployActions.js index 16496caf1f..c385cdbf33 100644 --- a/contracts/deploy/deployActions.js +++ b/contracts/deploy/deployActions.js @@ -1236,12 +1236,10 @@ const deploySafeModulesForUnitTests = async () => { addresses.dead, ]); - await deployWithConfirmation("MockClaimableStrategy"); await deployWithConfirmation("ClaimStrategyRewardsSafeModule", [ cSafeContract.address, // safe cSafeContract.address, // operator [], // strategies — added per-test via addStrategy - addresses.dead, // rewardsTo placeholder — overridden in tests via setRewardsTo ]); }; diff --git a/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js b/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js deleted file mode 100644 index 2945c59c62..0000000000 --- a/contracts/deploy/mainnet/187_claim_strategy_rewards_module_v2.js +++ /dev/null @@ -1,66 +0,0 @@ -const addresses = require("../../utils/addresses"); -const { deploymentWithGovernanceProposal } = require("../../utils/deploy"); -const { impersonateAndFund } = require("../../utils/signers"); -const { isFork } = require("../../utils/hardhat-helpers"); - -module.exports = deploymentWithGovernanceProposal( - { - deployName: "187_claim_strategy_rewards_module_v2", - forceDeploy: false, - reduceQueueTime: true, - deployerIsProposer: false, - proposalId: "", - }, - async ({ deployWithConfirmation, withConfirmation }) => { - const safeAddress = addresses.multichainStrategist; - - // Strategies whose reward tokens will be claimed and forwarded. - const cOUSDCurveAMOProxy = await ethers.getContract("OUSDCurveAMOProxy"); - const cOETHCurveAMOProxy = await ethers.getContract("OETHCurveAMOProxy"); - const cOUSDMorphoV2StrategyProxy = await ethers.getContract( - "OUSDMorphoV2StrategyProxy" - ); - - const strategyAddresses = [ - cOUSDCurveAMOProxy.address, - cOETHCurveAMOProxy.address, - cOUSDMorphoV2StrategyProxy.address, - ]; - - // Address that claimed reward tokens are forwarded to. - // Defaults to the multichainStrategist Safe — confirm before deploying. - const rewardsTo = addresses.mainnet.CoWHarvester; - - await deployWithConfirmation("ClaimStrategyRewardsSafeModule", [ - safeAddress, // safe - addresses.mainnet.validatorRegistrator, // operator (Defender relayer) - strategyAddresses, - rewardsTo, - ]); - const cModule = await ethers.getContract("ClaimStrategyRewardsSafeModule"); - - console.log( - `ClaimStrategyRewardsSafeModule (for ${safeAddress}) deployed to`, - cModule.address - ); - console.log(`rewardsTo set to ${rewardsTo}`); - - if (isFork) { - const safeSigner = await impersonateAndFund(safeAddress); - const cSafe = await ethers.getContractAt( - ["function enableModule(address module) external"], - safeAddress - ); - - await withConfirmation( - cSafe.connect(safeSigner).enableModule(cModule.address) - ); - - console.log("Enabled module on fork"); - } - - return { - actions: [], - }; - } -); 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..e494e3a93f --- /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.OUSD, // _otoken + addresses.mainnet.USDC, // _hardAsset + addresses.mainnet.curve.OUSD_USDC.gauge, // _gauge + addresses.mainnet.CRVMinter, // _minter + ], + "CurveAMOStrategy", // actual contract to compile + true // skipUpgradeSafety — storage layout change (harvesterPaused) is intentional + ); + + // 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 CoW Harvester on OUSD Curve AMO + { + contract: cOUSDCurveAMO, + signature: "setHarvesterAddress(address)", + args: [addresses.mainnet.CoWHarvester], + }, + // 3. Upgrade OETH Curve AMO + { + contract: cOETHCurveAMOProxy, + signature: "upgradeTo(address)", + args: [dOETHCurveAMO.address], + }, + // 4. Set CoW Harvester on OETH Curve AMO + { + contract: cOETHCurveAMO, + signature: "setHarvesterAddress(address)", + args: [addresses.mainnet.CoWHarvester], + }, + // 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/_fixture.js b/contracts/test/_fixture.js index 9bb05c8330..df49dabcd4 100644 --- a/contracts/test/_fixture.js +++ b/contracts/test/_fixture.js @@ -930,39 +930,6 @@ async function claimRewardsModuleFixture() { }; } -async function claimStrategyRewardsModuleFixture() { - const fixture = await defaultFixture(); - - const claimRewardsModule = await ethers.getContract( - "ClaimStrategyRewardsSafeModule" - ); - const mockSafe = await ethers.getContract("MockSafeContract"); - const mockClaimableStrategy = await ethers.getContract( - "MockClaimableStrategy" - ); - const mockUSDC = await ethers.getContract("MockUSDC"); - const mockDAI = await ethers.getContract("MockDAI"); - - // MockSafeContract is both safe and operator in the unit-test deployment. - const safeSigner = await impersonateAndFund(mockSafe.address); - - // A stranger with no roles - const stranger = await impersonateAndFund( - "0x0000000000000000000000000000000000000002" - ); - - return { - ...fixture, - claimRewardsModule, - mockSafe, - mockClaimableStrategy, - mockUSDC, - mockDAI, - safeSigner, - stranger, - }; -} - async function autoWithdrawalModuleFixture() { const fixture = await defaultFixture(); @@ -1872,7 +1839,6 @@ module.exports = { bridgeHelperModuleFixture, beaconChainFixture, claimRewardsModuleFixture, - claimStrategyRewardsModuleFixture, autoWithdrawalModuleFixture, crossChainFixtureUnit, crossChainFixture, diff --git a/contracts/test/behaviour/harvestable.js b/contracts/test/behaviour/harvestable.js index 714f611961..35c4776d42 100644 --- a/contracts/test/behaviour/harvestable.js +++ b/contracts/test/behaviour/harvestable.js @@ -22,13 +22,19 @@ const shouldBehaveLikeHarvestable = (context) => { const harvesterSigner = await impersonateAndFund(harvester.address); await strategy.connect(harvesterSigner).collectRewardTokens(); }); + + it("Should allow strategist to collect rewards", async () => { + const { strategist, strategy } = context(); + await strategy.connect(strategist).collectRewardTokens(); + }); + it("Should NOT allow rewards to be collected by non-harvester", async () => { - const { anna, governor, strategist, strategy } = context(); + const { anna, governor, strategy } = context(); - 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("Caller is not the Harvester or Strategist"); } }); @@ -44,6 +50,62 @@ const shouldBehaveLikeHarvestable = (context) => { ]) ).to.be.revertedWith("Can not set an empty address as a reward token"); }); + + describe("pauseHarvester / unpauseHarvester", () => { + it("Governor can pause and emits HarvesterPaused", async () => { + const { governor, strategy } = context(); + await expect(strategy.connect(governor).pauseHarvester()) + .to.emit(strategy, "HarvesterPaused") + .withArgs(governor.address); + expect(await strategy.harvesterPaused()).to.be.true; + }); + + it("Strategist can pause", async () => { + const { strategist, strategy } = context(); + await strategy.connect(strategist).pauseHarvester(); + expect(await strategy.harvesterPaused()).to.be.true; + }); + + it("Random address cannot pause", async () => { + const { anna, strategy } = context(); + await expect( + strategy.connect(anna).pauseHarvester() + ).to.be.revertedWith("Caller is not the Strategist or Governor"); + }); + + it("Governor can unpause and emits HarvesterUnpaused", async () => { + const { governor, strategy } = context(); + await strategy.connect(governor).pauseHarvester(); + await expect(strategy.connect(governor).unpauseHarvester()) + .to.emit(strategy, "HarvesterUnpaused") + .withArgs(governor.address); + expect(await strategy.harvesterPaused()).to.be.false; + }); + + it("Strategist cannot unpause", async () => { + const { governor, strategist, strategy } = context(); + await strategy.connect(governor).pauseHarvester(); + await expect( + strategy.connect(strategist).unpauseHarvester() + ).to.be.revertedWith("Caller is not the Governor"); + }); + + it("Random address cannot unpause", async () => { + const { governor, anna, strategy } = context(); + await strategy.connect(governor).pauseHarvester(); + await expect( + strategy.connect(anna).unpauseHarvester() + ).to.be.revertedWith("Caller is not the Governor"); + }); + + it("collectRewardTokens succeeds but skips transfer when paused", async () => { + const { governor, harvester, strategy } = context(); + await strategy.connect(governor).pauseHarvester(); + // Should not revert — call succeeds, just no-ops the transfer + const harvesterSigner = await impersonateAndFund(harvester.address); + await strategy.connect(harvesterSigner).collectRewardTokens(); + }); + }); }); }; diff --git a/contracts/test/safe-modules/claim-strategy-rewards.js b/contracts/test/safe-modules/claim-strategy-rewards.js deleted file mode 100644 index dc80db7e21..0000000000 --- a/contracts/test/safe-modules/claim-strategy-rewards.js +++ /dev/null @@ -1,381 +0,0 @@ -const { expect } = require("chai"); -const { - createFixtureLoader, - claimStrategyRewardsModuleFixture, -} = require("../_fixture"); -const addresses = require("../../utils/addresses"); -const { parseUnits } = require("ethers/lib/utils"); - -const fixture = createFixtureLoader(claimStrategyRewardsModuleFixture); - -describe("Unit Test: Claim Strategy Rewards Safe Module", function () { - let f; - let rewardsTo; - - beforeEach(async () => { - f = await fixture(); - // Use a deterministic test address as the rewards destination - rewardsTo = "0x0000000000000000000000000000000000000099"; - - // Set rewardsTo via the safe (admin) - await f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(rewardsTo); - }); - - // ─── Deployment ─────────────────────────────────────────────────────────── - - describe("Deployment", () => { - it("Should set safeContract", async () => { - expect(await f.claimRewardsModule.safeContract()).to.eq( - f.mockSafe.address - ); - }); - - it("Should set rewardsTo to addresses.dead initially", async () => { - // The freshly deployed contract has addresses.dead as rewardsTo - // (set in deploySafeModulesForUnitTests), but beforeEach overrides it. - // Verify the override took effect. - expect(await f.claimRewardsModule.rewardsTo()).to.eq(rewardsTo); - }); - }); - - // ─── setRewardsTo ───────────────────────────────────────────────────────── - - describe("setRewardsTo()", () => { - it("Should update rewardsTo and emit event", async () => { - const newAddress = "0x0000000000000000000000000000000000000042"; - await expect( - f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(newAddress) - ) - .to.emit(f.claimRewardsModule, "RewardsToUpdated") - .withArgs(newAddress); - - expect(await f.claimRewardsModule.rewardsTo()).to.eq(newAddress); - }); - - it("Should revert on zero address", async () => { - await expect( - f.claimRewardsModule.connect(f.safeSigner).setRewardsTo(addresses.zero) - ).to.be.revertedWith("Invalid rewardsTo address"); - }); - - it("Should revert if called by non-admin", async () => { - await expect( - f.claimRewardsModule.connect(f.stranger).setRewardsTo(rewardsTo) - ).to.be.reverted; - }); - }); - - // ─── addStrategy / removeStrategy ───────────────────────────────────────── - - describe("addStrategy() / removeStrategy()", () => { - it("Should add and whitelist a strategy", async () => { - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - expect( - await f.claimRewardsModule.isStrategyWhitelisted( - f.mockClaimableStrategy.address - ) - ).to.be.true; - }); - - it("Should revert when adding a duplicate strategy", async () => { - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - await expect( - f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address) - ).to.be.revertedWith("Strategy already whitelisted"); - }); - - it("Should remove a strategy", async () => { - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - await f.claimRewardsModule - .connect(f.safeSigner) - .removeStrategy(f.mockClaimableStrategy.address); - - expect( - await f.claimRewardsModule.isStrategyWhitelisted( - f.mockClaimableStrategy.address - ) - ).to.be.false; - }); - - it("Should revert when removing a non-whitelisted strategy", async () => { - await expect( - f.claimRewardsModule - .connect(f.safeSigner) - .removeStrategy(f.mockClaimableStrategy.address) - ).to.be.revertedWith("Strategy not whitelisted"); - }); - - it("Should revert if called by non-admin", async () => { - await expect( - f.claimRewardsModule - .connect(f.stranger) - .addStrategy(f.mockClaimableStrategy.address) - ).to.be.reverted; - }); - }); - - // ─── claimRewardsFor ────────────────────────────────────────────────────── - - describe("claimRewardsFor()", () => { - it("Should revert for non-whitelisted strategy", async () => { - await expect( - f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false) - ).to.be.revertedWith("Strategy not whitelisted"); - }); - - it("Should revert if called by non-operator", async () => { - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - await expect( - f.claimRewardsModule - .connect(f.stranger) - .claimRewardsFor(f.mockClaimableStrategy.address, false) - ).to.be.reverted; - }); - - it("Should claim and forward tokens to rewardsTo", async () => { - const { mockUSDC } = f; - const amount = parseUnits("100", 6); - - // Configure the strategy with a reward token and fund it - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - await mockUSDC.mintTo(f.mockClaimableStrategy.address, amount); - - // Whitelist the strategy - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - const rewardsToBalanceBefore = await mockUSDC.balanceOf(rewardsTo); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false); - - await expect(tx) - .to.emit(f.claimRewardsModule, "RewardTokensForwarded") - .withArgs(f.mockClaimableStrategy.address, mockUSDC.address, amount); - - // Tokens should arrive at rewardsTo, not remain in the Safe - expect(await mockUSDC.balanceOf(rewardsTo)).to.eq( - rewardsToBalanceBefore.add(amount) - ); - expect(await mockUSDC.balanceOf(f.mockSafe.address)).to.eq(0); - }); - - it("Should do nothing when strategy has no reward tokens", async () => { - await f.mockClaimableStrategy.setRewardTokenAddresses([]); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false); - - await expect(tx).to.not.emit( - f.claimRewardsModule, - "RewardTokensForwarded" - ); - }); - - it("Should do nothing when reward token balance is zero", async () => { - const { mockUSDC } = f; - - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - // No tokens minted to strategy - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false); - - await expect(tx).to.not.emit( - f.claimRewardsModule, - "RewardTokensForwarded" - ); - }); - - it("Should emit ClaimRewardsFailed and revert when silent=false", async () => { - const { mockUSDC } = f; - - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - await f.mockClaimableStrategy.setShouldRevert(true); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - await expect( - f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false) - ).to.be.revertedWith("Failed to claim rewards"); - }); - - it("Should emit ClaimRewardsFailed but not revert when silent=true", async () => { - const { mockUSDC } = f; - - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - await f.mockClaimableStrategy.setShouldRevert(true); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, true); - - await expect(tx) - .to.emit(f.claimRewardsModule, "ClaimRewardsFailed") - .withArgs(f.mockClaimableStrategy.address); - }); - - it("Should handle multiple reward tokens", async () => { - const { mockUSDC, mockDAI } = f; - const usdcAmount = parseUnits("50", 6); - const daiAmount = parseUnits("200", 18); - - await f.mockClaimableStrategy.setRewardTokenAddresses([ - mockUSDC.address, - mockDAI.address, - ]); - await mockUSDC.mintTo(f.mockClaimableStrategy.address, usdcAmount); - await mockDAI.mintTo(f.mockClaimableStrategy.address, daiAmount); - - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewardsFor(f.mockClaimableStrategy.address, false); - - await expect(tx) - .to.emit(f.claimRewardsModule, "RewardTokensForwarded") - .withArgs( - f.mockClaimableStrategy.address, - mockUSDC.address, - usdcAmount - ); - await expect(tx) - .to.emit(f.claimRewardsModule, "RewardTokensForwarded") - .withArgs(f.mockClaimableStrategy.address, mockDAI.address, daiAmount); - - expect(await mockUSDC.balanceOf(rewardsTo)).to.eq(usdcAmount); - expect(await mockDAI.balanceOf(rewardsTo)).to.eq(daiAmount); - }); - }); - - // ─── claimRewards (claimAll) ─────────────────────────────────────────────── - - describe("claimRewards() — claim all", () => { - it("Should revert if called by non-operator", async () => { - await expect(f.claimRewardsModule.connect(f.stranger).claimRewards(false)) - .to.be.reverted; - }); - - it("Should claim and forward from all whitelisted strategies", async () => { - const { mockUSDC, mockDAI } = f; - - // Deploy a second mock claimable strategy by repurposing the fixture one - // and using a second ethers.getContract with a different address. - // For simplicity, add the same strategy twice would fail, so we use - // two separate strategies by deploying a second one inline. - const MockClaimableStrategy = await ethers.getContractFactory( - "MockClaimableStrategy" - ); - const strategy2 = await MockClaimableStrategy.deploy(); - - const amount1 = parseUnits("100", 6); - const amount2 = parseUnits("300", 18); - - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - await mockUSDC.mintTo(f.mockClaimableStrategy.address, amount1); - - await strategy2.setRewardTokenAddresses([mockDAI.address]); - await mockDAI.mintTo(strategy2.address, amount2); - - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(strategy2.address); - - await f.claimRewardsModule.connect(f.safeSigner).claimRewards(false); - - expect(await mockUSDC.balanceOf(rewardsTo)).to.eq(amount1); - expect(await mockDAI.balanceOf(rewardsTo)).to.eq(amount2); - expect(await mockUSDC.balanceOf(f.mockSafe.address)).to.eq(0); - expect(await mockDAI.balanceOf(f.mockSafe.address)).to.eq(0); - }); - - it("Should continue past failures when silent=true", async () => { - const { mockUSDC, mockDAI } = f; - - const MockClaimableStrategy = await ethers.getContractFactory( - "MockClaimableStrategy" - ); - const strategy2 = await MockClaimableStrategy.deploy(); - - const amount2 = parseUnits("300", 18); - - // strategy1 will revert - await f.mockClaimableStrategy.setRewardTokenAddresses([mockUSDC.address]); - await f.mockClaimableStrategy.setShouldRevert(true); - - // strategy2 succeeds - await strategy2.setRewardTokenAddresses([mockDAI.address]); - await mockDAI.mintTo(strategy2.address, amount2); - - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(strategy2.address); - - const tx = await f.claimRewardsModule - .connect(f.safeSigner) - .claimRewards(true); - - await expect(tx) - .to.emit(f.claimRewardsModule, "ClaimRewardsFailed") - .withArgs(f.mockClaimableStrategy.address); - await expect(tx) - .to.emit(f.claimRewardsModule, "RewardTokensForwarded") - .withArgs(strategy2.address, mockDAI.address, amount2); - - expect(await mockDAI.balanceOf(rewardsTo)).to.eq(amount2); - }); - - it("Should stop on first failure when silent=false", async () => { - await f.mockClaimableStrategy.setRewardTokenAddresses([]); - await f.mockClaimableStrategy.setShouldRevert(true); - await f.claimRewardsModule - .connect(f.safeSigner) - .addStrategy(f.mockClaimableStrategy.address); - - await expect( - f.claimRewardsModule.connect(f.safeSigner).claimRewards(false) - ).to.be.revertedWith("Failed to claim rewards"); - }); - }); -}); 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..722e2c1a8a 100644 --- a/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js @@ -971,7 +971,7 @@ describe("Curve AMO OETH strategy", function () { governor: governor, oeth: oeth, harvester: harvester, - strategist: rafael, + strategist: impersonatedStrategist, })); const mintAndDepositToStrategy = async ({ @@ -1226,4 +1226,69 @@ describe("Curve AMO OETH strategy", function () { return profit; }; + + describe("Harvester pause and CoW Harvester", () => { + beforeEach(async () => { + // Simulate post-deploy state: set harvester to CoW Harvester + await curveAMOStrategy + .connect(impersonatedAMOGovernor) + .setHarvesterAddress(addresses.mainnet.CoWHarvester); + }); + + it("Should have CoW Harvester set as harvesterAddress", async () => { + expect(await curveAMOStrategy.harvesterAddress()).to.equal( + addresses.mainnet.CoWHarvester + ); + }); + + it("Strategist can call collectRewardTokens directly", async () => { + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + }); + + it("Should pause and stop reward transfers, then unpause to resume", async () => { + // Seed strategy with CRV + await setERC20TokenBalance(curveAMOStrategy.address, crv, "1000", hre); + + const cowHarvesterAddress = addresses.mainnet.CoWHarvester; + const cowBalBefore = await crv.balanceOf(cowHarvesterAddress); + + // Strategist pauses + await expect( + curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester() + ) + .to.emit(curveAMOStrategy, "HarvesterPaused") + .withArgs(impersonatedStrategist.address); + expect(await curveAMOStrategy.harvesterPaused()).to.be.true; + + // collectRewardTokens succeeds but rewards stay in strategy + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await crv.balanceOf(cowHarvesterAddress)).to.equal(cowBalBefore); + expect(await crv.balanceOf(curveAMOStrategy.address)).to.be.gt(0); + + // Governor unpauses + await expect( + curveAMOStrategy.connect(impersonatedAMOGovernor).unpauseHarvester() + ) + .to.emit(curveAMOStrategy, "HarvesterUnpaused") + .withArgs(impersonatedAMOGovernor.address); + expect(await curveAMOStrategy.harvesterPaused()).to.be.false; + + // Next collect sends CRV to CoW Harvester + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await crv.balanceOf(cowHarvesterAddress)).to.be.gt(cowBalBefore); + }); + + it("Strategist cannot unpause", async () => { + await curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester(); + await expect( + curveAMOStrategy.connect(impersonatedStrategist).unpauseHarvester() + ).to.be.revertedWith("Caller is not the Governor"); + }); + }); }); 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..f6167ebf88 100644 --- a/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js @@ -975,7 +975,7 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { governor: governor, oeth: ousd, harvester: harvester, - strategist: rafael, + strategist: impersonatedStrategist, })); const mintAndDepositToStrategy = async ({ @@ -1239,4 +1239,69 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { return profit; }; + + describe("Harvester pause and CoW Harvester", () => { + beforeEach(async () => { + // Simulate post-deploy state: set harvester to CoW Harvester + await curveAMOStrategy + .connect(impersonatedAMOGovernor) + .setHarvesterAddress(addresses.mainnet.CoWHarvester); + }); + + it("Should have CoW Harvester set as harvesterAddress", async () => { + expect(await curveAMOStrategy.harvesterAddress()).to.equal( + addresses.mainnet.CoWHarvester + ); + }); + + it("Strategist can call collectRewardTokens directly", async () => { + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + }); + + it("Should pause and stop reward transfers, then unpause to resume", async () => { + // Seed strategy with CRV + await setERC20TokenBalance(curveAMOStrategy.address, crv, "1000", hre); + + const cowHarvesterAddress = addresses.mainnet.CoWHarvester; + const cowBalBefore = await crv.balanceOf(cowHarvesterAddress); + + // Strategist pauses + await expect( + curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester() + ) + .to.emit(curveAMOStrategy, "HarvesterPaused") + .withArgs(impersonatedStrategist.address); + expect(await curveAMOStrategy.harvesterPaused()).to.be.true; + + // collectRewardTokens succeeds but rewards stay in strategy + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await crv.balanceOf(cowHarvesterAddress)).to.equal(cowBalBefore); + expect(await crv.balanceOf(curveAMOStrategy.address)).to.be.gt(0); + + // Governor unpauses + await expect( + curveAMOStrategy.connect(impersonatedAMOGovernor).unpauseHarvester() + ) + .to.emit(curveAMOStrategy, "HarvesterUnpaused") + .withArgs(impersonatedAMOGovernor.address); + expect(await curveAMOStrategy.harvesterPaused()).to.be.false; + + // Next collect sends CRV to CoW Harvester + await curveAMOStrategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await crv.balanceOf(cowHarvesterAddress)).to.be.gt(cowBalBefore); + }); + + it("Strategist cannot unpause", async () => { + await curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester(); + await expect( + curveAMOStrategy.connect(impersonatedStrategist).unpauseHarvester() + ).to.be.revertedWith("Caller is not the Governor"); + }); + }); }); 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..ee1baf0f89 100644 --- a/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js +++ b/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js @@ -8,6 +8,8 @@ const { units, isCI } = require("../helpers"); const { createFixtureLoader, morphoOUSDv2Fixture } = require("../_fixture"); const { impersonateAndFund } = require("../../utils/signers"); +const { setERC20TokenBalance } = require("../_fund"); +const hre = require("hardhat"); const log = require("../../utils/logger")("test:fork:ousd-v2-morpho"); @@ -414,4 +416,100 @@ describe("ForkTest: Yearn's Morpho OUSD v2 Strategy", function () { } }); }); + + describe("Harvester pause and CoW Harvester", () => { + const loadFixture = createFixtureLoader(morphoOUSDv2Fixture); + let impersonatedGov, impersonatedStrategist; + + beforeEach(async () => { + fixture = await loadFixture(); + const { morphoOUSDv2Strategy } = fixture; + + impersonatedGov = await impersonateAndFund( + await morphoOUSDv2Strategy.governor() + ); + impersonatedStrategist = await impersonateAndFund( + addresses.multichainStrategist + ); + + // Simulate post-deploy state: set harvester to CoW Harvester + await morphoOUSDv2Strategy + .connect(impersonatedGov) + .setHarvesterAddress(addresses.mainnet.CoWHarvester); + }); + + it("Should have CoW Harvester set as harvesterAddress", async () => { + const { morphoOUSDv2Strategy } = fixture; + expect(await morphoOUSDv2Strategy.harvesterAddress()).to.equal( + addresses.mainnet.CoWHarvester + ); + }); + + it("Strategist can call collectRewardTokens directly", async () => { + const { morphoOUSDv2Strategy } = fixture; + await morphoOUSDv2Strategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + }); + + it("Should pause and stop reward transfers, then unpause to resume", async () => { + const { morphoOUSDv2Strategy, morphoToken } = fixture; + + // Seed strategy with MORPHO tokens + await setERC20TokenBalance( + morphoOUSDv2Strategy.address, + morphoToken, + "1000", + hre + ); + + const cowHarvesterAddress = addresses.mainnet.CoWHarvester; + const cowBalBefore = await morphoToken.balanceOf(cowHarvesterAddress); + + // Strategist pauses + await expect( + morphoOUSDv2Strategy.connect(impersonatedStrategist).pauseHarvester() + ) + .to.emit(morphoOUSDv2Strategy, "HarvesterPaused") + .withArgs(impersonatedStrategist.address); + expect(await morphoOUSDv2Strategy.harvesterPaused()).to.be.true; + + // collectRewardTokens succeeds but rewards stay in strategy + await morphoOUSDv2Strategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await morphoToken.balanceOf(cowHarvesterAddress)).to.equal( + cowBalBefore + ); + expect( + await morphoToken.balanceOf(morphoOUSDv2Strategy.address) + ).to.be.gt(0); + + // Governor unpauses + await expect( + morphoOUSDv2Strategy.connect(impersonatedGov).unpauseHarvester() + ) + .to.emit(morphoOUSDv2Strategy, "HarvesterUnpaused") + .withArgs(impersonatedGov.address); + expect(await morphoOUSDv2Strategy.harvesterPaused()).to.be.false; + + // Next collect sends MORPHO to CoW Harvester + await morphoOUSDv2Strategy + .connect(impersonatedStrategist) + .collectRewardTokens(); + expect(await morphoToken.balanceOf(cowHarvesterAddress)).to.be.gt( + cowBalBefore + ); + }); + + it("Strategist cannot unpause", async () => { + const { morphoOUSDv2Strategy } = fixture; + await morphoOUSDv2Strategy + .connect(impersonatedStrategist) + .pauseHarvester(); + await expect( + morphoOUSDv2Strategy.connect(impersonatedStrategist).unpauseHarvester() + ).to.be.revertedWith("Caller is not the Governor"); + }); + }); }); From ae8876e3941e5360177f9ac5a650a2352f954bb1 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Mon, 30 Mar 2026 17:24:00 +0400 Subject: [PATCH 4/9] Fix CI and remove pause functionality --- contracts/contracts/interfaces/IStrategy.sol | 6 -- .../utils/InitializableAbstractStrategy.sol | 42 +------- .../187_upgrade_curve_amo_morpho_harvester.js | 12 +-- contracts/test/behaviour/harvestable.js | 56 ----------- .../curve-amo-oeth.mainnet.fork-test.js | 65 ------------ .../curve-amo-ousd.mainnet.fork-test.js | 65 ------------ .../ousd-v2-morpho.mainnet.fork-test.js | 98 ------------------- 7 files changed, 11 insertions(+), 333 deletions(-) diff --git a/contracts/contracts/interfaces/IStrategy.sol b/contracts/contracts/interfaces/IStrategy.sol index 3ea9f546d1..f02f0f7689 100644 --- a/contracts/contracts/interfaces/IStrategy.sol +++ b/contracts/contracts/interfaces/IStrategy.sol @@ -57,12 +57,6 @@ interface IStrategy { function harvesterAddress() external view returns (address); - function harvesterPaused() external view returns (bool); - - function pauseHarvester() external; - - function unpauseHarvester() external; - function transferToken(address token, uint256 amount) external; function setRewardTokenAddresses(address[] calldata _rewardTokenAddresses) diff --git a/contracts/contracts/utils/InitializableAbstractStrategy.sol b/contracts/contracts/utils/InitializableAbstractStrategy.sol index a862e887b6..82b74b759d 100644 --- a/contracts/contracts/utils/InitializableAbstractStrategy.sol +++ b/contracts/contracts/utils/InitializableAbstractStrategy.sol @@ -32,8 +32,6 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { address _oldHarvesterAddress, address _newHarvesterAddress ); - event HarvesterPaused(address pausedBy); - event HarvesterUnpaused(address unpausedBy); /// @notice Address of the underlying platform address public immutable platformAddress; @@ -68,17 +66,12 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /// @notice Address of the reward tokens. eg CRV, BAL, CVX, AURA address[] public rewardTokenAddresses; - /// @notice Whether the harvester is paused. When true, collectRewardTokens - /// will not transfer rewards to the harvester address. - bool public harvesterPaused; - /* Reserved for future expansion. Used to be 100 storage slots * and has decreased to accommodate: * - harvesterAddress * - rewardTokenAddresses - * - harvesterPaused */ - int256[97] private _reserved; + int256[98] private _reserved; struct BaseStrategyConfig { address platformAddress; // Address of the underlying platform @@ -126,11 +119,10 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /** * @notice Collect accumulated reward token and send to Vault. - * No-ops when the harvester is not set or is paused — rewards - * remain in the strategy until the harvester is unpaused or reset. + * No-ops when the harvester address is not set. */ function collectRewardTokens() external virtual onlyHarvester nonReentrant { - if (harvesterAddress == address(0) || harvesterPaused) { + if (harvesterAddress == address(0)) { return; } _collectRewardTokens(); @@ -141,10 +133,7 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { * Implementing strategies need to add custom logic to collect the rewards. */ function _collectRewardTokens() internal virtual { - // The harvester check is intentionally repeated here (it also exists in the - // external collectRewardTokens) because strategies may override the external - // function without calling super, bypassing that check. - if (harvesterAddress == address(0) || harvesterPaused) { + if (harvesterAddress == address(0)) { return; } uint256 rewardTokenCount = rewardTokenAddresses.length; @@ -317,33 +306,12 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { */ function setHarvesterAddress(address _harvesterAddress) external - onlyGovernor + onlyGovernorOrStrategist { emit HarvesterAddressesUpdated(harvesterAddress, _harvesterAddress); harvesterAddress = _harvesterAddress; } - /** - * @notice Pause reward token transfers to the Harvester. - * Callable by the Strategist for a fast response to a rogue harvester. - * While paused, collectRewardTokens() still runs but rewards remain - * in the strategy contract. - */ - function pauseHarvester() external onlyGovernorOrStrategist { - harvesterPaused = true; - emit HarvesterPaused(msg.sender); - } - - /** - * @notice Unpause reward token transfers to the Harvester. - * Governor only — intended to be called after setting a new - * harvester address via setHarvesterAddress(). - */ - function unpauseHarvester() external onlyGovernor { - harvesterPaused = false; - emit HarvesterUnpaused(msg.sender); - } - /*************************************** Abstract ****************************************/ diff --git a/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js b/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js index e494e3a93f..b8102bba0c 100644 --- a/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js +++ b/contracts/deploy/mainnet/187_upgrade_curve_amo_morpho_harvester.js @@ -43,13 +43,13 @@ module.exports = deploymentWithGovernanceProposal( addresses.mainnet.curve.OUSD_USDC.pool, // platformAddress (Curve pool = LP token) addresses.mainnet.VaultProxy, // vaultAddress ], - addresses.mainnet.OUSD, // _otoken + 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 // skipUpgradeSafety — storage layout change (harvesterPaused) is intentional + true ); // 2. OETH/WETH Curve AMO @@ -93,11 +93,11 @@ module.exports = deploymentWithGovernanceProposal( signature: "upgradeTo(address)", args: [dOUSDCurveAMO.address], }, - // 2. Set CoW Harvester on OUSD Curve AMO + // 2. Set Multichain Strategist on OUSD Curve AMO { contract: cOUSDCurveAMO, signature: "setHarvesterAddress(address)", - args: [addresses.mainnet.CoWHarvester], + args: [addresses.multichainStrategist], }, // 3. Upgrade OETH Curve AMO { @@ -105,11 +105,11 @@ module.exports = deploymentWithGovernanceProposal( signature: "upgradeTo(address)", args: [dOETHCurveAMO.address], }, - // 4. Set CoW Harvester on OETH Curve AMO + // 4. Set Multichain Strategist on OETH Curve AMO { contract: cOETHCurveAMO, signature: "setHarvesterAddress(address)", - args: [addresses.mainnet.CoWHarvester], + args: [addresses.multichainStrategist], }, // 5. Upgrade OUSD Morpho V2 { diff --git a/contracts/test/behaviour/harvestable.js b/contracts/test/behaviour/harvestable.js index 35c4776d42..ecbd32e3e5 100644 --- a/contracts/test/behaviour/harvestable.js +++ b/contracts/test/behaviour/harvestable.js @@ -50,62 +50,6 @@ const shouldBehaveLikeHarvestable = (context) => { ]) ).to.be.revertedWith("Can not set an empty address as a reward token"); }); - - describe("pauseHarvester / unpauseHarvester", () => { - it("Governor can pause and emits HarvesterPaused", async () => { - const { governor, strategy } = context(); - await expect(strategy.connect(governor).pauseHarvester()) - .to.emit(strategy, "HarvesterPaused") - .withArgs(governor.address); - expect(await strategy.harvesterPaused()).to.be.true; - }); - - it("Strategist can pause", async () => { - const { strategist, strategy } = context(); - await strategy.connect(strategist).pauseHarvester(); - expect(await strategy.harvesterPaused()).to.be.true; - }); - - it("Random address cannot pause", async () => { - const { anna, strategy } = context(); - await expect( - strategy.connect(anna).pauseHarvester() - ).to.be.revertedWith("Caller is not the Strategist or Governor"); - }); - - it("Governor can unpause and emits HarvesterUnpaused", async () => { - const { governor, strategy } = context(); - await strategy.connect(governor).pauseHarvester(); - await expect(strategy.connect(governor).unpauseHarvester()) - .to.emit(strategy, "HarvesterUnpaused") - .withArgs(governor.address); - expect(await strategy.harvesterPaused()).to.be.false; - }); - - it("Strategist cannot unpause", async () => { - const { governor, strategist, strategy } = context(); - await strategy.connect(governor).pauseHarvester(); - await expect( - strategy.connect(strategist).unpauseHarvester() - ).to.be.revertedWith("Caller is not the Governor"); - }); - - it("Random address cannot unpause", async () => { - const { governor, anna, strategy } = context(); - await strategy.connect(governor).pauseHarvester(); - await expect( - strategy.connect(anna).unpauseHarvester() - ).to.be.revertedWith("Caller is not the Governor"); - }); - - it("collectRewardTokens succeeds but skips transfer when paused", async () => { - const { governor, harvester, strategy } = context(); - await strategy.connect(governor).pauseHarvester(); - // Should not revert — call succeeds, just no-ops the transfer - const harvesterSigner = await impersonateAndFund(harvester.address); - await strategy.connect(harvesterSigner).collectRewardTokens(); - }); - }); }); }; 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 722e2c1a8a..0f65839f1e 100644 --- a/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js @@ -1226,69 +1226,4 @@ describe("Curve AMO OETH strategy", function () { return profit; }; - - describe("Harvester pause and CoW Harvester", () => { - beforeEach(async () => { - // Simulate post-deploy state: set harvester to CoW Harvester - await curveAMOStrategy - .connect(impersonatedAMOGovernor) - .setHarvesterAddress(addresses.mainnet.CoWHarvester); - }); - - it("Should have CoW Harvester set as harvesterAddress", async () => { - expect(await curveAMOStrategy.harvesterAddress()).to.equal( - addresses.mainnet.CoWHarvester - ); - }); - - it("Strategist can call collectRewardTokens directly", async () => { - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - }); - - it("Should pause and stop reward transfers, then unpause to resume", async () => { - // Seed strategy with CRV - await setERC20TokenBalance(curveAMOStrategy.address, crv, "1000", hre); - - const cowHarvesterAddress = addresses.mainnet.CoWHarvester; - const cowBalBefore = await crv.balanceOf(cowHarvesterAddress); - - // Strategist pauses - await expect( - curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester() - ) - .to.emit(curveAMOStrategy, "HarvesterPaused") - .withArgs(impersonatedStrategist.address); - expect(await curveAMOStrategy.harvesterPaused()).to.be.true; - - // collectRewardTokens succeeds but rewards stay in strategy - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await crv.balanceOf(cowHarvesterAddress)).to.equal(cowBalBefore); - expect(await crv.balanceOf(curveAMOStrategy.address)).to.be.gt(0); - - // Governor unpauses - await expect( - curveAMOStrategy.connect(impersonatedAMOGovernor).unpauseHarvester() - ) - .to.emit(curveAMOStrategy, "HarvesterUnpaused") - .withArgs(impersonatedAMOGovernor.address); - expect(await curveAMOStrategy.harvesterPaused()).to.be.false; - - // Next collect sends CRV to CoW Harvester - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await crv.balanceOf(cowHarvesterAddress)).to.be.gt(cowBalBefore); - }); - - it("Strategist cannot unpause", async () => { - await curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester(); - await expect( - curveAMOStrategy.connect(impersonatedStrategist).unpauseHarvester() - ).to.be.revertedWith("Caller is not the Governor"); - }); - }); }); 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 f6167ebf88..3608843973 100644 --- a/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js +++ b/contracts/test/strategies/curve-amo-ousd.mainnet.fork-test.js @@ -1239,69 +1239,4 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { return profit; }; - - describe("Harvester pause and CoW Harvester", () => { - beforeEach(async () => { - // Simulate post-deploy state: set harvester to CoW Harvester - await curveAMOStrategy - .connect(impersonatedAMOGovernor) - .setHarvesterAddress(addresses.mainnet.CoWHarvester); - }); - - it("Should have CoW Harvester set as harvesterAddress", async () => { - expect(await curveAMOStrategy.harvesterAddress()).to.equal( - addresses.mainnet.CoWHarvester - ); - }); - - it("Strategist can call collectRewardTokens directly", async () => { - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - }); - - it("Should pause and stop reward transfers, then unpause to resume", async () => { - // Seed strategy with CRV - await setERC20TokenBalance(curveAMOStrategy.address, crv, "1000", hre); - - const cowHarvesterAddress = addresses.mainnet.CoWHarvester; - const cowBalBefore = await crv.balanceOf(cowHarvesterAddress); - - // Strategist pauses - await expect( - curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester() - ) - .to.emit(curveAMOStrategy, "HarvesterPaused") - .withArgs(impersonatedStrategist.address); - expect(await curveAMOStrategy.harvesterPaused()).to.be.true; - - // collectRewardTokens succeeds but rewards stay in strategy - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await crv.balanceOf(cowHarvesterAddress)).to.equal(cowBalBefore); - expect(await crv.balanceOf(curveAMOStrategy.address)).to.be.gt(0); - - // Governor unpauses - await expect( - curveAMOStrategy.connect(impersonatedAMOGovernor).unpauseHarvester() - ) - .to.emit(curveAMOStrategy, "HarvesterUnpaused") - .withArgs(impersonatedAMOGovernor.address); - expect(await curveAMOStrategy.harvesterPaused()).to.be.false; - - // Next collect sends CRV to CoW Harvester - await curveAMOStrategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await crv.balanceOf(cowHarvesterAddress)).to.be.gt(cowBalBefore); - }); - - it("Strategist cannot unpause", async () => { - await curveAMOStrategy.connect(impersonatedStrategist).pauseHarvester(); - await expect( - curveAMOStrategy.connect(impersonatedStrategist).unpauseHarvester() - ).to.be.revertedWith("Caller is not the Governor"); - }); - }); }); 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 ee1baf0f89..22279dc9a3 100644 --- a/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js +++ b/contracts/test/strategies/ousd-v2-morpho.mainnet.fork-test.js @@ -8,8 +8,6 @@ const { units, isCI } = require("../helpers"); const { createFixtureLoader, morphoOUSDv2Fixture } = require("../_fixture"); const { impersonateAndFund } = require("../../utils/signers"); -const { setERC20TokenBalance } = require("../_fund"); -const hre = require("hardhat"); const log = require("../../utils/logger")("test:fork:ousd-v2-morpho"); @@ -416,100 +414,4 @@ describe("ForkTest: Yearn's Morpho OUSD v2 Strategy", function () { } }); }); - - describe("Harvester pause and CoW Harvester", () => { - const loadFixture = createFixtureLoader(morphoOUSDv2Fixture); - let impersonatedGov, impersonatedStrategist; - - beforeEach(async () => { - fixture = await loadFixture(); - const { morphoOUSDv2Strategy } = fixture; - - impersonatedGov = await impersonateAndFund( - await morphoOUSDv2Strategy.governor() - ); - impersonatedStrategist = await impersonateAndFund( - addresses.multichainStrategist - ); - - // Simulate post-deploy state: set harvester to CoW Harvester - await morphoOUSDv2Strategy - .connect(impersonatedGov) - .setHarvesterAddress(addresses.mainnet.CoWHarvester); - }); - - it("Should have CoW Harvester set as harvesterAddress", async () => { - const { morphoOUSDv2Strategy } = fixture; - expect(await morphoOUSDv2Strategy.harvesterAddress()).to.equal( - addresses.mainnet.CoWHarvester - ); - }); - - it("Strategist can call collectRewardTokens directly", async () => { - const { morphoOUSDv2Strategy } = fixture; - await morphoOUSDv2Strategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - }); - - it("Should pause and stop reward transfers, then unpause to resume", async () => { - const { morphoOUSDv2Strategy, morphoToken } = fixture; - - // Seed strategy with MORPHO tokens - await setERC20TokenBalance( - morphoOUSDv2Strategy.address, - morphoToken, - "1000", - hre - ); - - const cowHarvesterAddress = addresses.mainnet.CoWHarvester; - const cowBalBefore = await morphoToken.balanceOf(cowHarvesterAddress); - - // Strategist pauses - await expect( - morphoOUSDv2Strategy.connect(impersonatedStrategist).pauseHarvester() - ) - .to.emit(morphoOUSDv2Strategy, "HarvesterPaused") - .withArgs(impersonatedStrategist.address); - expect(await morphoOUSDv2Strategy.harvesterPaused()).to.be.true; - - // collectRewardTokens succeeds but rewards stay in strategy - await morphoOUSDv2Strategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await morphoToken.balanceOf(cowHarvesterAddress)).to.equal( - cowBalBefore - ); - expect( - await morphoToken.balanceOf(morphoOUSDv2Strategy.address) - ).to.be.gt(0); - - // Governor unpauses - await expect( - morphoOUSDv2Strategy.connect(impersonatedGov).unpauseHarvester() - ) - .to.emit(morphoOUSDv2Strategy, "HarvesterUnpaused") - .withArgs(impersonatedGov.address); - expect(await morphoOUSDv2Strategy.harvesterPaused()).to.be.false; - - // Next collect sends MORPHO to CoW Harvester - await morphoOUSDv2Strategy - .connect(impersonatedStrategist) - .collectRewardTokens(); - expect(await morphoToken.balanceOf(cowHarvesterAddress)).to.be.gt( - cowBalBefore - ); - }); - - it("Strategist cannot unpause", async () => { - const { morphoOUSDv2Strategy } = fixture; - await morphoOUSDv2Strategy - .connect(impersonatedStrategist) - .pauseHarvester(); - await expect( - morphoOUSDv2Strategy.connect(impersonatedStrategist).unpauseHarvester() - ).to.be.revertedWith("Caller is not the Governor"); - }); - }); }); From 692241e071aa947171da7676789648383d250cf4 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Mon, 30 Mar 2026 17:40:28 +0400 Subject: [PATCH 5/9] Fix failing tests --- contracts/test/behaviour/strategy.js | 6 +++--- .../test/strategies/ousd-v2-morpho.mainnet.fork-test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/test/behaviour/strategy.js b/contracts/test/behaviour/strategy.js index 23880954cb..3e0b16e8d0 100644 --- a/contracts/test/behaviour/strategy.js +++ b/contracts/test/behaviour/strategy.js @@ -403,16 +403,16 @@ 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 { strategy, 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]) { + for (const signer of [matt, harvesterSigner, vaultSigner]) { await expect( strategy.connect(signer).setHarvesterAddress(randomAddress) - ).to.revertedWith("Caller is not the Governor"); + ).to.revertedWith("Caller is not the Strategist or Governor"); } }); it("Should allow reward tokens to be set by the governor", async () => { 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 () => { From f7f8438cd2b692281a7d6eb87683319dbd562d1c Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Mon, 30 Mar 2026 19:15:11 +0400 Subject: [PATCH 6/9] fix tests --- contracts/test/behaviour/harvestable.js | 15 ++++++++--- contracts/test/behaviour/strategy.js | 27 ++++++++++++++----- .../curve-amo-oeth.mainnet.fork-test.js | 2 ++ .../curve-amo-ousd.mainnet.fork-test.js | 2 ++ 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/contracts/test/behaviour/harvestable.js b/contracts/test/behaviour/harvestable.js index ecbd32e3e5..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, })); */ @@ -24,17 +26,22 @@ const shouldBehaveLikeHarvestable = (context) => { }); it("Should allow strategist to collect rewards", async () => { - const { strategist, strategy } = context(); + 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, 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]) { await expect( strategy.connect(signer).collectRewardTokens() - ).to.be.revertedWith("Caller is not the Harvester or Strategist"); + ).to.be.revertedWith(errMsg); } }); diff --git a/contracts/test/behaviour/strategy.js b/contracts/test/behaviour/strategy.js index 3e0b16e8d0..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, 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 [matt, harvesterSigner, vaultSigner]) { - await expect( - strategy.connect(signer).setHarvesterAddress(randomAddress) - ).to.revertedWith("Caller is not the Strategist or 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/curve-amo-oeth.mainnet.fork-test.js b/contracts/test/strategies/curve-amo-oeth.mainnet.fork-test.js index 0f65839f1e..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(); @@ -972,6 +973,7 @@ describe("Curve AMO OETH strategy", function () { oeth: oeth, harvester: harvester, 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 3608843973..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(); @@ -976,6 +977,7 @@ describe("Fork Test: Curve AMO OUSD strategy", function () { oeth: ousd, harvester: harvester, strategist: impersonatedStrategist, + newBehavior: true, })); const mintAndDepositToStrategy = async ({ From 79d1676efc8209ed559f8cf64266732a1c04fa6c Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Tue, 31 Mar 2026 20:40:35 +0400 Subject: [PATCH 7/9] Rename modifier --- contracts/contracts/strategies/BaseCurveAMOStrategy.sol | 2 +- contracts/contracts/strategies/CurveAMOStrategy.sol | 2 +- .../contracts/strategies/algebra/StableSwapAMMStrategy.sol | 2 +- .../strategies/crosschain/CrossChainMasterStrategy.sol | 2 +- contracts/contracts/utils/InitializableAbstractStrategy.sol | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) 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 82b74b759d..9c441677b3 100644 --- a/contracts/contracts/utils/InitializableAbstractStrategy.sol +++ b/contracts/contracts/utils/InitializableAbstractStrategy.sol @@ -162,7 +162,7 @@ abstract contract InitializableAbstractStrategy is Initializable, Governable { /** * @dev Verifies that the caller is the Harvester or Strategist. */ - modifier onlyHarvester() { + modifier onlyHarvesterOrStrategist() { require( msg.sender == harvesterAddress || msg.sender == IVault(vaultAddress).strategistAddr(), From 4bb3bf41fd49c30e3f524ec4cf6e3c1a67f60631 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Wed, 1 Apr 2026 12:30:16 +0400 Subject: [PATCH 8/9] Fix renamed modifier --- .../contracts/utils/InitializableAbstractStrategy.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/contracts/utils/InitializableAbstractStrategy.sol b/contracts/contracts/utils/InitializableAbstractStrategy.sol index 9c441677b3..3873f72143 100644 --- a/contracts/contracts/utils/InitializableAbstractStrategy.sol +++ b/contracts/contracts/utils/InitializableAbstractStrategy.sol @@ -121,7 +121,12 @@ 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; } From bca69e16e13e8d1b86ce261dd395c92620c6f0d3 Mon Sep 17 00:00:00 2001 From: Shahul Hameed <10547529+shahthepro@users.noreply.github.com> Date: Wed, 1 Apr 2026 13:07:21 +0400 Subject: [PATCH 9/9] Fix unit tests --- CLAUDE.md | 1 + contracts/test/strategies/compoundingSSVStaking.js | 1 + contracts/test/strategies/nativeSSVStaking.js | 2 ++ 3 files changed, 4 insertions(+) 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/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/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 () {