[fix] - fixing convex strategy rewards selling

[SHAKOTN]

Done:

1. Strategy doesn't sell rewards in case of emergencyMode is set to true
2. setAdditionalRewards is fixed and tested
3. All curve pools are non-constants now
4. More tests

By completing all from the above, we make sure that no matter the situation with Curve, we can keep strategies running and withdraw rewards without selling them

[codecov[bot]]

Codecov Report

Merging #106 (055eda2) into master (e1a67e0) will increase coverage by 3.10%. The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   71.80%   74.91%   +3.10%     
==========================================
  Files          15       15              
  Lines        1199     1228      +29     
  Branches      268      272       +4     
==========================================
+ Hits          861      920      +59     
+ Misses        226      187      -39     
- Partials      112      121       +9     

Files Changed	Coverage Δ
contracts/strategy/ConvexStrategy.sol	76.61% <86.20%> (+11.13%)	:arrow_up:

... and 5 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[UsmannK]

Hey, @kitty-the-kat asked me to take a look at this PR.

Mostly LGTM, seems like main changes were to make the pool addresses variable and allow for manual reward sweeping/redemption in the case of an emergency.

Two things I noticed:

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L747-L756

The withdraw() function attempts to sell all rewards unconditional of the emergency status. Is this intended, or should a gate on emergency mode be added here?

---

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L479-L501

The set*Pool functions do not seem to set approvals to the new pools or revoke approvals to the old pools.

In the constructor this is done for the original pools:

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L322-L335

Perhaps the tests should check that the strategy continues to function after setting pools? It seems like it might not. Some light fuzzing could be helpful.

Will continue looking at this tomorrow but I don't think I will find anything else notable. Let me know if I misunderstood the above though.

[SHAKOTN]

Hey, @kitty-the-kat asked me to take a look at this PR.

Mostly LGTM, seems like main changes were to make the pool addresses variable and allow for manual reward sweeping/redemption in the case of an emergency.

Two things I noticed:

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L747-L756

The withdraw() function attempts to sell all rewards unconditional of the emergency status. Is this intended, or should a gate on emergency mode be added here?

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L479-L501

The set*Pool functions do not seem to set approvals to the new pools or revoke approvals to the old pools.

In the constructor this is done for the original pools:

https://github.com/groLabs/GSquared/blob/3dba1a288eebf92bb30c3c561b77d9ff57c6eda1/contracts/strategy/ConvexStrategy.sol#L322-L335

Perhaps the tests should check that the strategy continues to function after setting pools? It seems like it might not. Some light fuzzing could be helpful.

Will continue looking at this tomorrow but I don't think I will find anything else notable. Let me know if I misunderstood the above though.

Thanks for the review!

1. I moved emergencyMode flag check inside the sellAllRewards function so now we won't sell rewards in case strategy is in emergency in all places
2. I've added allowances setting everywhere and additional tests to check out that they work
3. Some additional tests were added to check that strategy can be harvested and is selling rewards after we set new curve pool

[UsmannK]

New changes look good to me, previously mentioned issues are addressed. Left one nit regarding a comment.