updates after security review

contracts/StrategyCurveTricrypto.sol

@@ -54,6 +54,7 @@ abstract contract StrategyCurveBase is BaseStrategy {
     // keepCRV stuff
     uint256 public keepCRV; // the percentage of CRV we re-lock for boost (in basis points)
     uint256 internal constant FEE_DENOMINATOR = 10000; // this means all of our fee values are in basis points
+    uint256 internal constant MIN_REWARD_TO_SELL = 1e18; // minimum amount required of CRV to attempt a sell.
 
     IERC20 public constant crv =
         IERC20(0x1E4F97b9f9F913c46F1632781732927B9019C68b);
@@ -133,8 +134,28 @@ abstract contract StrategyCurveBase is BaseStrategy {
         return balanceOfWant();
     }
 
+    function claimRewards() internal returns (uint256) {
+        // Claims any pending CRV and returns the total CRV balance for the strategy
+        //
+        // Mint claimable CRV from the factory gauge. The old claim_rewards() function now
+        // only applies to third-party tokens
+        gaugeFactory.mint(address(gauge));
+
+        return crv.balanceOf(address(this));
+    }
+
     function prepareMigration(address _newStrategy) internal override {
+        // Claim any pending rewards and transfer CRV balance to the new strategy
+        uint256 _crvBalance = claimRewards();
+
+        if (_crvBalance > 0) {
+            crv.safeTransfer(_newStrategy, _crvBalance);
+        }
+
+        // Withdraw LP tokens from the gauge. The transfer to the new strategy is done
+        // by migrate() in BaseStrategy.sol
         uint256 _stakedBal = stakedBalance();
+
         if (_stakedBal > 0) {
             gauge.withdraw(_stakedBal);
         }
@@ -228,24 +249,23 @@ contract StrategyCurveTricrypto is StrategyCurveBase {
             uint256 _debtPayment
         )
     {
-    // Mint claimable CRV from the factory gauge. The old claim_rewards() function now only applies to third-party tokens
-    gaugeFactory.mint(address(gauge));
+        // Claim and get a fresh snapshot of the strategy's CRV balance
+        uint256 _crvBalance = claimRewards();
 
-        uint256 crvBalance = crv.balanceOf(address(this));
-        // if we claimed any CRV, then sell it
-        if (crvBalance > 0) {
+        // Sell CRV if we have any
+        if (_crvBalance > MIN_REWARD_TO_SELL) {
             // keep some of our CRV to increase our boost
-            uint256 sendToVoter = crvBalance.mul(keepCRV).div(FEE_DENOMINATOR);
+            uint256 sendToVoter = _crvBalance.mul(keepCRV).div(FEE_DENOMINATOR);
             if (keepCRV > 0) {
                 crv.safeTransfer(voter, sendToVoter);
             }
 
             // check our balance again after transferring some crv to our voter
-            crvBalance = crv.balanceOf(address(this));
+            _crvBalance = crv.balanceOf(address(this));
 
             // sell the rest of our CRV
-            if (crvBalance > 0) {
-                _sellToken(address(crv), crvBalance);
+            if (_crvBalance > 0) {
+                _sellToken(address(crv), _crvBalance);
             }
         }
 

contracts/interfaces/curve.sol

@@ -11,7 +11,7 @@ interface IGauge {
 
     function claim_rewards() external;
 
-    function claimable_tokens(address) external view returns (uint256);
+    function claimable_tokens(address) external returns (uint256);
 
     function claimable_reward(address _addressToCheck, address _rewardToken)
         external

tests/test_migration.py

@@ -18,6 +18,7 @@ def test_migration(
     pool,
     strategy_name,
     gauge,
+    crv,
 ):
 
     ## deposit to the vault after approving
@@ -45,11 +46,25 @@ def test_migration(
     chain.sleep(86400)
     chain.mine(1)
 
+    claimable_tokens = gauge.claimable_tokens.call(strategy)
+    crv_balance_old_strat = crv.balanceOf(strategy)
+
+    assert claimable_tokens > 0, "No tokens to be claimed"
+
     # migrate our old strategy
     vault.migrateStrategy(strategy, new_strategy, {"from": gov})
     new_strategy.setHealthCheck(healthCheck, {"from": gov})
     new_strategy.setDoHealthCheck(True, {"from": gov})
 
+    migrated_crv = crv.balanceOf(new_strategy)
+
+    assert migrated_crv >= (
+        claimable_tokens + crv_balance_old_strat
+    ), "Not everything transfered as expected"
+
+    claimable_tokens = gauge.claimable_tokens.call(strategy)
+    assert claimable_tokens == 0, "Left rewards behind"
+
     # assert that our old strategy is empty
     updated_total_old = strategy.estimatedTotalAssets()
     assert updated_total_old == 0
@@ -64,8 +79,8 @@ def test_migration(
         new_strat_balance, total_old, abs_tol=5
     )
 
-    startingVault = vault.totalAssets()
-    print("\nVault starting assets with new strategy: ", startingVault)
+    starting_vault_assets = vault.totalAssets()
+    print("\nVault starting assets with new strategy: ", starting_vault_assets)
 
     # simulate one day of earnings
     chain.sleep(86400)
@@ -75,8 +90,8 @@ def test_migration(
     new_strategy.harvest({"from": gov})
     vaultAssets_2 = vault.totalAssets()
     # confirm we made money, or at least that we have about the same
-    assert vaultAssets_2 >= startingVault or math.isclose(
-        vaultAssets_2, startingVault, abs_tol=5
+    assert vaultAssets_2 >= starting_vault_assets or math.isclose(
+        vaultAssets_2, starting_vault_assets, abs_tol=5
     )
     print("\nAssets after 1 day harvest: ", vaultAssets_2)
 
@@ -105,7 +120,6 @@ def test_migration_from_real_strat(
 
     # migrate our old strategy
     vaultDeployed.migrateStrategy(strategy_to_migrate_from, new_strategy, {"from": gov})
-    new_strategy.setHealthCheck(healthCheck, {"from": gov})
     new_strategy.setDoHealthCheck(True, {"from": gov})
 
     # assert that our old strategy is empty
@@ -119,8 +133,8 @@ def test_migration_from_real_strat(
     # confirm the same amount of assets were moved to the new strat
     assert new_strat_balance == total_old
 
-    startingVault = vaultDeployed.totalAssets()
-    print("\nVault starting assets with new strategy: ", startingVault)
+    starting_vault_assets = vaultDeployed.totalAssets()
+    print("\nVault starting assets with new strategy: ", starting_vault_assets)
 
     # simulate one day of earnings
     chain.sleep(86400)
@@ -131,4 +145,4 @@ def test_migration_from_real_strat(
     vaultAssets_2 = vaultDeployed.totalAssets()
 
     # confirm we made money
-    assert vaultAssets_2 > startingVault
+    assert vaultAssets_2 > starting_vault_assets

tests/test_odds_and_ends.py

@@ -75,8 +75,8 @@ def test_odds_and_ends(
     new_strat_balance = new_strategy.estimatedTotalAssets()
     assert new_strat_balance >= total_old
 
-    startingVault = vault.totalAssets()
-    print("\nVault starting assets with new strategy: ", startingVault)
+    starting_vault_assets = vault.totalAssets()
+    print("\nVault starting assets with new strategy: ", starting_vault_assets)
 
     # simulate one day of earnings
     chain.sleep(86400)
@@ -85,7 +85,7 @@ def test_odds_and_ends(
     # Test out our migrated strategy, confirm we're making a profit
     new_strategy.harvest({"from": gov})
     vaultAssets_2 = vault.totalAssets()
-    assert vaultAssets_2 >= startingVault
+    assert vaultAssets_2 >= starting_vault_assets
     print("\nAssets after 1 day harvest: ", vaultAssets_2)
 
     # check our oracle
@@ -201,8 +201,8 @@ def test_odds_and_ends_migration(
         new_strat_balance, total_old, abs_tol=5
     )
 
-    startingVault = vault.totalAssets()
-    print("\nVault starting assets with new strategy: ", startingVault)
+    starting_vault_assets = vault.totalAssets()
+    print("\nVault starting assets with new strategy: ", starting_vault_assets)
 
     # simulate one day of earnings
     chain.sleep(86400)
@@ -216,8 +216,8 @@ def test_odds_and_ends_migration(
     new_strategy.harvest({"from": gov})
     vaultAssets_2 = vault.totalAssets()
     # confirm we made money, or at least that we have about the same
-    assert vaultAssets_2 >= startingVault or math.isclose(
-        vaultAssets_2, startingVault, abs_tol=5
+    assert vaultAssets_2 >= starting_vault_assets or math.isclose(
+        vaultAssets_2, starting_vault_assets, abs_tol=5
     )
     print("\nAssets after 1 day harvest: ", vaultAssets_2)