-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Frax Convex Strategy with locked Frax Staked Convex LP tokens #1917
Frax Convex Strategy with locked Frax Staked Convex LP tokens #1917
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## nicka/convex-frxeth-weth-libs #1917 +/- ##
================================================================
Coverage ? 74.09%
================================================================
Files ? 61
Lines ? 3015
Branches ? 781
================================================================
Hits ? 2234
Misses ? 778
Partials ? 3 ☔ View full report in Codecov by Sentry. |
…nto nicka/convex-frax-locking
It's possible we could make this far simpler. Let's say that we have an internal method
This means that all our locking/unlocking logic now lives in one function, and rather than having four methods that have to do separate logic and calls about funds into and out of locking. |
Added fraxConvexWethFixture Added fxs to default fixture Added fork tests for FraxConvex strategy using the frxETH/WETH Curve pool
…nto nicka/convex-frax-locking
I like this approach. It can also work for the function that rolls the lock. I got the basic fork tests working today. I'll work on cleaning things up tomorrow and more testing |
Yeah, it should do the lock rolling as well. One thing I said incorrectly was that unlocking should reduce the pendingWithdraws - it should actually be withdrawing that does that, since otherwise the funds would get locked up again on the next invocation. |
I've refactored the There are multiple levels of tokens managed by this strategy:
Note there are no partial withdrawals of expired locked tokens so full withdraws of expired tokens are done before locking more. Below is what the high level design now looks like
I'm now working on the required permutation for the fork tests |
Lock event emitted if only time was added. The amount is zero. Small gas optimization to updateLock to avoid second SLOAD
…nicka/convex-frax-locking
if (lockedBalance < targetLockedBalanceMem) { | ||
// Calculate the amount of Frax Staked Convex LP tokens to lock. | ||
// The target lock balance ignoring the unlocked balance | ||
uint256 targetLockAmount = targetLockedBalanceMem - lockedBalance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use unchecked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only part of the strategies that is gas critical is the balance checking (since it's used in every rebase). The rest of the strategy code is used only a few times a week, and I'd rather stay out of super gas optimizations in them, and lean towards safety.
* This will revert if there is not enough FXS rewards in the | ||
* Frax Convex Locking contract. See dev note in contract Natspec for more details. | ||
*/ | ||
function _lpWithdrawAll() internal override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The withdrawAll()
strategy might encounter failure if the withdrawLocked()
function from IFraxConvexLocking
reverts due to a paused withdrawal. This issue can block the withdrawal and unwrapping of funds that are not locked, subsequently preventing their transfer back to the vault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good point. I think it makes sense to wrap the _unlock
in _lpWithdrawAll
in a try catch so the unlocked LP tokens can still be withdrawn.
* @return balance Total value of the asset in the platform | ||
*/ | ||
function checkBalance(address _asset) | ||
public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function can be set to external.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still the case you get a gas saving for declaring a function external
? Especially for a view function.
When I changed checkBalance
on FraxConvexStrategy
to external I didn't see a gas saving from when it was public.
Doing some quick testing in Remix with the following also shows no gas difference between the functions being declared external
or public
. What am I missing?
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
contract ExternalFuncs {
address asset = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
uint256 balance = 1e18;
function checkBalance(address _asset) public view returns (uint256) {
require(_asset == asset, "invalid asset");
return balance;
}
function updateAsset(address _asset) public {
asset = _asset;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gas savings are no longer available; the purpose is now solely for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put this in PR #1976
What is the rationale behind the 7-day lock period? Edit: |
nonReentrant | ||
{ | ||
// Collect CRV, CVX and FXS rewards from staking contract | ||
IFraxConvexStaking(fraxStaking).getReward(address(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getReward()
function can be invoked using the harvester address, allowing for a reduction in three transfers (CRV, CVX, FXS) for both the fraxStaking
and fraxLocking
contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, so Frax allow rewards to be delegated. In that case, the strategy can delegate rewards to the Harvester.
This requires a change to the Harvester which would be out of scope for this deployment. I'll add this Harvester feature request to the backlog
uint256 targetLockedBalanceMem = targetLockedBalance; | ||
|
||
// Do not extend lock time or add funds if already over the target locked balance | ||
if (lockedBalance > targetLockedBalanceMem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an extreme side case when targetLockedBalanceMem
and lockedBalance
are both zero, it should probably return on line 204 too. Adding a || targetLockedBalanceMem == 0
will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid case - especially after withdrawAll
.
Since there are no locks and the lock amount is zero, _lock
will exit on line 226. But I like your suggestion of returning earlier by checking if the target locked balance is zero.
I'll add to a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Added Balancer deposit Update Convex AMO harvest Added Balancer harvest * Updated Convex AMO and Balancer harvests value flows Updated buyback value flows * Added vote locked CVX to buyback
More analysis is needed on how long the lock should be for. |
You could probably create multiple strategies with different durations. |
* Fix some tests * Fix some more * A couple more fixes * Fix balanced metapool fixture * Fix collateral swaps
* Added extra fork test to measure Frax Convex strategy's checkBalance gas usage * exit _lock function earlier if target and locked balances are both zero * Changed checkBalance on FraxConvexStrategy to external to make it clear it is not used internally.
* N-01 added indexed to Lock and Unlock events * Restore prettier format
@@ -143,7 +143,7 @@ contract ThreePoolStrategy is BaseCurveStrategy, CurveThreeCoinFunctions { | |||
} | |||
|
|||
/** | |||
* @dev Collect accumulated CRV and send to Vault. | |||
* @dev Collect accumulated CRV rewards and send to the Harvester. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for catching these mistakes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal Code Review
[This is not yet a complete review - still need to check the tests]
Deployment Considerations
- creates a new price feed
- updates the OracleRouter with the new price feed
- sets new OracleRouter as new OETHVault price provider
- deploys a new strategy proxy
- deploys a new strategy implementation
- adds Uniswap config to sell FXS to the Harvester. Correct 1% FXS / ETH pool is configured
🟢 all above steps are correct and expected
Attack
This will never be a default strategy so the attacks to look for would be pool manipulating ones. And as long as we use vaultValue checker with strategist moves that check the expected values (inflated checkBalance would be noticed) manipulations shouldn't be possible
Logic
🟢 Are there bugs in the logic?
🟢 Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).
🟢 Do public/external functions having asset parameter verify the asset is supported (where applicable)
Tests
🟢 Each publicly callable method has a test
🟢 Each logical branch has a test
🟢 Each require() has a test
🟢 Edge conditions are tested
🟢 If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.
Flavor
🟢 Could this code be simpler?
🟢 Could this code be less vulnerable to other code behaving weirdly?
Overflow
🟢 Never use "+" or "-", always use safe math or have contract compile in solidity version > 0.8
Check that all for loops use uint256
Proxy
🟢 Make sure proxy implementation contracts don't initialize variable state on variable declaration and do it rather in initialize function.
Black magic
🟢 Does not contain selfdestruct
🟢 Does not use delegatecall outside of proxying
(If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.)
Address.isContract should be treated as if could return anything at any time, because that's reality.
Dependencies
🟢 Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
If OpenZeppelin ACL roles are use review & enumerate all of them.
Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.
Deploy
🟢 Check that any deployer permissions are removed after deploy
Authentication
🟢 Never use tx.origin
🟢 Check that every external/public function should actually be external
🟢 Check that every external/public function has the correct authentication
Cryptographic code
Contracts that roll their own crypto are terrifying
Note that a failed signature check will result in a 0x00 result. Make sure that the result throws if it returns this.
~~ Beware of signed data being used in a replay attack to other contracts.~~
Gas problems
Contracts with for loops must have either:
A way to remove items
🟢Can be upgraded to get unstuck
Size can only controlled by admins
🟢 Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.
External calls
🟢 Contract addresses passed in are validated
⚪ No unsafe external calls
🟠 Reentrancy guards on all state changing functions
🟢 Still doesn't protect against external contracts changing the state of the world if they are called.
Malicious behaviors
🟢 Could fail from stack depth problems (low level calls must require success)
🟢 No slippage attacks (we need to validate expected tokens received)
Oracles, one of:
No oracles
⚪ Oracles can't be bent
⚪ If oracle can be bent, it won't hurt us.
🟢 Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?
Ethereum
🟢 Contract does not send or receive Ethereum.
🟢 Contract has no payable methods.
* @param _pTokens Address of the Curve pool for each asset. | ||
*/ | ||
function initialize( | ||
address[] calldata _rewardTokenAddresses, // CRV + CVX + FXS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a mental note. We never update these properties in the initialiser. Even if we needed them to we could withdrawAll from the strategy and deploy a new one.
It is worth considering altering InitializableAbstractStrategy and making all the configuration immutable. That will yield some nice gas savings not only within the strategy, but also rebase
and allocation
calls
Have created a discussion point on the next contract sync.
address[] calldata _assets, | ||
address[] calldata _pTokens | ||
) external onlyGovernor initializer { | ||
require( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could use custom error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also maybe add a comment:
currently all the assets in the curve pool need to be supported. Otherwise checkBalance
would return incorrect results
* @dev Verifies that the caller is the Strategist. | ||
*/ | ||
modifier onlyStrategist() { | ||
require( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: custom error.
lockKey = NO_KEY; | ||
|
||
// This strategy only supports assets with the same number of decimals | ||
require(decimals0 == decimals1, "Decimals do not match"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this check would fit better in the constructor rather than the initialiser, since all the data for it is already present there.
Also we could use a custom error
// This strategy only supports assets with the same number of decimals | ||
require(decimals0 == decimals1, "Decimals do not match"); | ||
if (CURVE_POOL_ASSETS_COUNT == 3) { | ||
require(decimals1 == decimals2, "Decimals do not match"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check also fits better in the constructor and also could use a custom error
// If no new lock is required | ||
if (lockAmount == 0) { | ||
// exit early so the unlockTimestamp is not updated or Lock event emitted | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this line of code only happen when there some targetLockedBalance
exits but the unlockedBalance
== 0 while no active lock exists? Meaning the contract has no balance at all to lock up. That seems more like an erroneous state to me than a legitimate one, and maybe it'd be better to throw an Error?
override | ||
returns (uint256 balance) | ||
{ | ||
require(_curveSupportedCoin(_asset), "Unsupported asset"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could use custom error
// get_virtual_price is gas intensive, so only call it if we have LP tokens. | ||
// Convert the Curve LP tokens controlled by this strategy to a value in USD or ETH | ||
uint256 value = (curveLpTokens * | ||
ICurvePool(CURVE_POOL).get_virtual_price()) / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about this... This will never be a default strategy so such attack is not feasible. But if it were, is the virtual price susceptible to read-only re-entrancy if the underlying pool has ETH?
@@ -79,6 +79,24 @@ async function constructNewContract(fixture, implContractName) { | |||
addresses.mainnet.WETH, | |||
], | |||
]; | |||
} else if (implContractName === "FraxConvexStrategy") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great to see hot deploys are being used!
* This will revert if there is not enough FXS rewards in the | ||
* Frax Convex Locking contract. See dev note in contract Natspec for more details. | ||
*/ | ||
function updateLock() external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it sounds a bit paranoid, but I would add nonReentrant
here
This PR #1981 fixes a test that had to be skipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really amazing test coverage of the convex frax strategy 💪
nit: There are so many different cases of locked, unlocked, staked, target locked amount variations it might be helpful to add a legend in the comments of the various cases covered.
wethBalanceExpected, | ||
0.01 // 0.01% or 1 basis point | ||
); | ||
// Check the frxETH balances in the Curve pool has no gone up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update comment. In case of depositing frxETH
it does go up
const withdrawAmount = "1000"; | ||
const withdrawAmountScaled = parseUnits(withdrawAmount); | ||
const expectedLpAmount = | ||
await fraxConvexWethStrategy.calcWithdrawLpAmount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential error in this contract function would also result in an incorrect expectedLpAmount
. I guess this should be fine as long as the correctness of calcWithdrawLpAmount
is tested somewhere else.
fraxConvexLockedWeth, | ||
} = fixture; | ||
|
||
// Get the ETH and OETH balances in the Curve Metapool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment
}; | ||
|
||
// Calculate the WETH and frxETH amounts from a withdrawAll | ||
async function calcWithdrawAllAmounts() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests the withhdrawAll once funds are deployed on the mainnet. The fork doesn't itself deploy any funds into it
const wethWithdrawAmount = curveBalances[0] | ||
.mul(strategyLpAmount) | ||
.div(totalLpSupply); | ||
// frxETH to burn = frxETH pool balance * strategy LP amount / total pool LP amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: frxEth to withdraw
) | ||
).to.equal(0); | ||
expect(await fraxConvexWethStrategy.lockKey()).to.equal(ONE_BYTES32); | ||
expect(await fraxConvexWethStrategy.unlockTimestamp()).to.equal(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above 3 lines will probably fail when mainnet state will have some locked amounts in the strategy.
A clean / empty strategy would probably require a fixture that waits for the lock period and then calls withdrawAll
on the strategy to empty it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixture config might require something like cleanup: false/true
aside from depositToStrategy: false
that would:
- wait for the duration of lockedPeriod
- harvest the rewards
- withdraw all the funds
|
||
const unlockTimestampBefore = | ||
await fraxConvexWethStrategy.unlockTimestamp(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we explicitly make the node pass some time, to express that current node's timestamp is greater than the locked timestamp
.to.emit(fraxConvexWethStrategy, "Lock") | ||
.withNamedArgs({ amount: 0 }); | ||
}); | ||
it("should deposit amount", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another test here would be useful that shows that updating the lock after depositing the funds does result in increased locked amount. We could test for when:
- deposited amount + existing locked balance < target locked balance
- and: deposited amount + existing locked balance > target locked balance
And see that in both cases the expected amounts are locked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha nvm you covered this with test cases below
// add to staked amount | ||
// no change to locked amounts | ||
await assertDeposit(parseUnits("333.33"), { | ||
unlockTimestamp: unlockTimestampBefore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could check that lockedBalance
matches unexpiredAmount.add(stakedAmount).add(parseUnits("333.33")
// add to staked amount | ||
// no change to locked amounts | ||
await assertDepositAll(parseUnits("0.001"), { | ||
unlockTimestamp: unlockTimestampBefore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could check for amounts here as well
Closing this PR because we are moving to Simplified OETH, without other LSTs as backing assets. |
This PR extends PR #1846 which implemented a Convex Strategy. This PR adds a
FraxConvexStrategy
that supports locking Frax Staked Convex LP tokens.There are multiple levels of tokens managed by this strategy
Contracts
The new strategy is in green. Modified contracts are in orange.
Value Flows
Deposit and locking flows
Harvesting CRV, CVX and FXS token rewards
Tests
There are no unit tests for the
FraxConvexStrategy
as it has heavy integration to Frax Convex.There is, however, a unit tests for
ConvexTwoPoolStrategy
which implementsConvexStrategy
which implementsBaseCurveStrategy
.FraxConvexStrategy
implementsBaseCurveStrategy
so there is some base unit test coverage.Fork tests
Deploy
The
contracts/deploy/082_frax_convex_strategy.js
script will deploy the new strategy and propose the required governance actions.Review
If you made a contract change, make sure to complete the checklist below before merging it in master.
Refer to our documentation for more details about contract security best practices.
Contract change checklist: