From 4044c0f677d4023aba1c96900ec54476aca860ed Mon Sep 17 00:00:00 2001 From: "pata.eth" Date: Fri, 20 May 2022 19:01:28 -0400 Subject: [PATCH] updates after security review --- contracts/StrategyCurveTricrypto.sol | 38 +++++++++++++++++++++------- contracts/interfaces/curve.sol | 2 +- tests/test_migration.py | 30 ++++++++++++++++------ tests/test_odds_and_ends.py | 14 +++++----- 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/contracts/StrategyCurveTricrypto.sol b/contracts/StrategyCurveTricrypto.sol index 7ecbdbf..d026600 100644 --- a/contracts/StrategyCurveTricrypto.sol +++ b/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); } } diff --git a/contracts/interfaces/curve.sol b/contracts/interfaces/curve.sol index 858de37..6f28662 100644 --- a/contracts/interfaces/curve.sol +++ b/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 diff --git a/tests/test_migration.py b/tests/test_migration.py index ef53526..954a867 100644 --- a/tests/test_migration.py +++ b/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 diff --git a/tests/test_odds_and_ends.py b/tests/test_odds_and_ends.py index e8952dc..7dd912b 100644 --- a/tests/test_odds_and_ends.py +++ b/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)