Uniswap V3 Strategy #1258

shahthepro · 2023-03-02T10:49:59Z

Things to decide:

decide if we are ok Strategist being 2/8 is having so much control over strategy configuration. 1 possible solution is to set the UniswapStrategist to be the old 5/8 governance. So we can change things quickly (no timelock) and still be relatively safe.

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:

Code reviewed by 2 reviewers.
Copy & paste code review security checklist below this checklist.
Unit tests pass
Slither tests pass with no warning
Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

sparrowDom · 2023-03-07T10:21:34Z

contracts/contracts/vault/VaultAdmin.sol

+    function _approveStrategy(address _addr, bool isUniswapV3) internal {
        require(!strategies[_addr].isSupported, "Strategy already approved");
-        strategies[_addr] = Strategy({ isSupported: true, _deprecated: 0 });
+        strategies[_addr] = Strategy({


Leaving a note here: Strategy structs are mapped via:

mapping(address => Strategy) internal strategies

the [bool, uint256, bool] cause the Strategy structure to take up 3 storage slots. The new variable isUniswapV3Strategy has correctly been added to the end of the struct, ensuring that no old mappings are affected.

nitpick: in theory because of a struct taking up 3 storage slots there could be collisions but it is really really highly unlikely.

DanielVF · 2023-03-07T13:33:55Z

Security goals

From a security design point of view, we should assume that the operator account is completely controlled by North Korea a couple times each year.

One style of attack would be for a malicious operator to almost infinitely bend the pool (a feature of Uniswap v3), then use the operator to rebalance to a tick near that bent position. The amount would be set to pull all USDC from the matching reserve strategy on that side into the position to be lost. When the attacker trades back through that new position, they would pick up our USDC for basically free. The attack would then repeat on the other side of pool to get all of our USDT.

We must have a way to cap the maximum damage that a malicious operator can do, it must be at the contract level, and it must be able to cover all operator operations.

We also need to be able to disable the operator immediately from the strategist multisig. Don't want to have to wait seven days for governance.

Gas usage

A major component of the profitability of this strategy is likely the gas costs. We don't have to go all ultra MEV bot assembly level, but we do need to know what are gas usage is, and make sure we aren't doing anything silly. One thing this might rule out is any safety checks at the vault total value level, since that's crazy expensive.

Swap

It looks like we may need a way to swap from one coin to another to build our position. Let's say we only have USDC, and we need a mix of USDC and USDT to go into active position.

DanielVF · 2023-03-07T13:43:08Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+        if (selfBalance < amount) {
+            // Try to pull remaining amount from reserve strategy
+            // This might throw if there isn't enough in reserve strategy as well
+            IVault(vaultAddress).withdrawForUniswapV3(


These strategies are already under the vault's control, and could be withdrawn directly from. The part that the vault can't get to is the uniswap position. We'd need to withdraw from the currently active position instead. This might give us some extra coins on the other side, but that would be okay. They can sit on the strategy, because the vault might be asking for them next.

Got it, will change that part of the code

we probably won't make Uniswap an asset default strategy right?

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

DanielVF · 2023-03-07T13:53:01Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+                "Invalid token"
+            );
+
+            (uint256 amount0, uint256 amount1) = uniswapV3Helper.positionValue(


Calculating a strategies balance is the place where gas efficiency matters most. We go through every coin, in every strategy, every rebase, redeem, large mint. And we do it at least twice during allocations.

Given that the operator is probably rebalancing every day or few, it's might be worth considering how much extra gas taking into account fees in this calculation will cost us.

Our rule for strategy balance checking is that an external user should not be able to influence the prices up and then down again. If I'm understanding what this is doing (calculating how much we could get for a withdraw), this would seem to allow that, though only at the level of inside our tick range.

Our usual rule for curve strategies is to always return the worst case funds return. This could also be more gas efficient here.

I'll run some tests on fork to see how much the gas diff is

DanielVF · 2023-03-07T14:00:40Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+    function _depositAll() internal {
+        uint256 token0Bal = IERC20(token0).balanceOf(address(this));
+        uint256 token1Bal = IERC20(token1).balanceOf(address(this));
+        if (token0Bal > 0) {


This could actually be greater than zero. If depositing dust costs $20 in gas, we do it once a day, and we earn 3% on the dust, then my math (which may be wrong) says we would still be better off keeping up to $240,000 dust in a Strat. This may be excessive, but something like $40K dust should be fine.

Agreed, I guess we could also probably make it a configurable variable, if we wanna tweak it in future

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

DanielVF · 2023-03-07T14:05:47Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+            uint256 amount1
+        )
+    {
+        INonfungiblePositionManager.MintParams memory params = INonfungiblePositionManager


Should we check that we aren't already at this tick range? Otherwise we could clobber our previous position id and lose track of it? Or make a note in the comments of the method?

I do know we are checking this elsewhere, but it always scares me when a method does something dangerous, perhaps not obvious, but the check is outside the method.

DanielVF · 2023-03-07T14:06:29Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+                tickUpper: upperTick,
+                amount0Desired: desiredAmount0,
+                amount1Desired: desiredAmount1,
+                amount0Min: maxSlippage == 0


minimum amounts should be passed in from the operator. Fixed percentage slippages leave far too much open for abuse.

was thinking of the same thing. The operator should have a clear view of the state of Uniswap pool without the dangers of front running bots altering it just before the transaction executes. It would make sense to pass the min amounts as parameters to the external function

sparrowDom

Security
Similar to what @DanielVF points out my biggest fear is that Uniswap strategy gives the Operator the ability to override the Governance allocation and pull all the funds of the reserve strategies for token0 & token1 assets.

One way to mitigate the issue would be to keep internal accounting of how much of each asset has been deposited to reserve strategies and how much withdrawn. So Uniswap strategy could never withdraw more than it has deposited.

Another way would be to have a strategy TVL threshold that gets updated every time allocation is called. This way when Vault would be withdrawingForUniswapV3 it would be able to check that uniswapV3.checkBlalance() + assetRequested < uniswapV3TVL

Rebalance approaches

Might be cool that we test out some more rebalancing approaches:

if we end up with much more of one token than another we could simulate various approaches to rebalancing:
- Swapping those tokens to have 50/50 share of tokens and rebalancing
- Deploying the overhead of the tokens into reserve strategy
- Deploying liquidity in imbalanced manner (the way strategy tokens are imbalanced)
have we though about copying (with doing sanity checks) Sommelier or other successful rebalancing protocols?

contracts/contracts/proxies/Proxies.sol

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

sparrowDom · 2023-03-07T14:11:34Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+            _vaultAddress,
+            new address[](0), // No Reward tokens
+            _assets, // Asset addresses
+            _assets // Platform token addresses


Do we need to set pTokens? Real pTokens on Uniswap are NFTs right, might be ok to just leave this as an empty array.

pTokens aren't needed for this strategy, yeah. InitializableAbstractStrategy.initialize() would throw if I pass a empty array here. Couldn't pass address(0) either since _setPTokenAddress() (called in initialize() method) would throw.

I thought of copying relevant code from InitializableAbstractStrategy and getting rid of pToken and rewardToken logics but that'd involve a lot of code duplication and might be harder to maintain in the long run. So, as of now, the Uniswap strategy just ignores the pToken and rewardTokenAddresses fields

sparrowDom · 2023-03-07T14:16:53Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+     * @param _token0ReserveStrategy The new reserve strategy for token0
+     * @param _token1ReserveStrategy The new reserve strategy for token1
+     */
+    function _setReserveStrategy(


nit: It would make more sense to me for this setter to have such signature:

function _setReserveStrategy( address asset // equals either token0 or token1 by its values address _assetReserveStrategy ) internal { ....

so you can set only one of the reserves at a time

sparrowDom · 2023-03-07T14:36:16Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+        if (selfBalance < amount) {
+            // Try to pull remaining amount from reserve strategy
+            // This might throw if there isn't enough in reserve strategy as well
+            IVault(vaultAddress).withdrawForUniswapV3(


we probably won't make Uniswap an asset default strategy right?

sparrowDom · 2023-03-07T14:45:25Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+        // Withdraw enough funds from Reserve strategies
+        uint256 token0Balance = cToken0.balanceOf(address(this));
+        if (token0Balance < minAmount0) {
+            vault.withdrawForUniswapV3(


Should we keep track of how much UniswapV3 is allowed to pull from reserve strategies? As in the max amount of liquidity that has been assigned to it by the weekly allocation?

sparrowDom · 2023-03-07T15:21:13Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+                tickUpper: upperTick,
+                amount0Desired: desiredAmount0,
+                amount1Desired: desiredAmount1,
+                amount0Min: maxSlippage == 0


was thinking of the same thing. The operator should have a clear view of the state of Uniswap pool without the dangers of front running bots altering it just before the transaction executes. It would make sense to pass the min amounts as parameters to the external function

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

sparrowDom · 2023-03-07T15:24:03Z

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol

+        uint256,
+        bytes calldata
+    ) external returns (bytes4) {
+        // TODO: Should we reject unwanted NFTs being transfered to the strategy?


is there any downside to leaving unexpected NFTs on the contract?

None as far as I can tell, but just wanted to know if anyone else thinks otherwise

sparrowDom · 2023-03-07T15:43:15Z

Also awesome work on this massive PR!

shahthepro · 2023-03-08T12:24:28Z

Thanks for the comments @DanielVF @sparrowDom

I have locally made the changes to remove the maxSlippage and made it as an passable arg. I'll also add a threshold on the amount of tokens Uniswap Strategy can pull from reserve strategies

* Remove unused imports * Add back import

* N1 - Code simplification * Constants to upper case * N04 - Indexing event params * N05 - Fix comments * N12 - typos * N09 - Simplify require statements * N-08 - Remove storage gap * N-07 - Naming * N-06 - Naming of internal functions & variables * Fix comment

codecov · 2023-06-06T12:04:00Z

Codecov Report

Merging #1258 (042a32b) into master (af63e4d) will increase coverage by 2.88%.
The diff coverage is 86.99%.

❗ Current head 042a32b differs from pull request most recent head 5d4063d. Consider uploading reports for the commit 5d4063d to get more accurate results

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   68.81%   71.69%   +2.88%     
==========================================
  Files          43       47       +4     
  Lines        2312     2784     +472     
  Branches      607      737     +130     
==========================================
+ Hits         1591     1996     +405     
- Misses        718      785      +67     
  Partials        3        3

Impacted Files	Coverage Δ
contracts/contracts/governance/Governable.sol	`100.00% <ø> (ø)`
...contracts/strategies/uniswap/UniswapV3Strategy.sol	`84.31% <84.31%> (ø)`
contracts/contracts/vault/VaultAdmin.sol	`94.47% <86.11%> (-0.74%)`	⬇️
...s/strategies/uniswap/UniswapV3LiquidityManager.sol	`86.44% <86.44%> (ø)`
contracts/contracts/utils/UniswapV3Helper.sol	`90.00% <90.00%> (ø)`
...ts/strategies/uniswap/UniswapV3StrategyStorage.sol	`100.00% <100.00%> (ø)`
.../contracts/utils/InitializableAbstractStrategy.sol	`88.40% <100.00%> (ø)`
contracts/contracts/vault/VaultCore.sol	`97.92% <100.00%> (+<0.01%)`	⬆️
contracts/contracts/vault/VaultStorage.sol	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

naddison36

I've done a first pass of the code. I haven't gone deep into the logic yet.

contracts/contracts/utils/UniswapV3Helper.sol

naddison36 · 2023-06-23T00:25:26Z

contracts/contracts/utils/UniswapV3Helper.sol

+ *      So, the libraries cannot be directly imported into OUSD contracts.
+ *      This contract (on v0.7.6) just proxies the calls to the Uniswap Libraries.
+ */
+contract UniswapV3Helper {


These are all pure functions so I would make this a library. That will also be cheaper on gas as the helper address does not have to be read from storage.

naddison36 · 2023-06-23T00:27:28Z

contracts/contracts/strategies/uniswap/UniswapV3StrategyStorage.sol

+
+    // The address that can manage the positions on Uniswap V3
+    address public operatorAddr;
+    address public token0; // Token0 of Uniswap V3 Pool


any storage variable that can't be changed should be an immutable. Especially frequently used variables like this one.

naddison36 · 2023-06-23T00:29:12Z

contracts/contracts/strategies/uniswap/UniswapV3StrategyStorage.sol

+    uint256 public maxPositionValueLostThreshold;
+
+    // Uniswap V3's Pool
+    IUniswapV3Pool public pool;


this can be immutable

naddison36 · 2023-06-23T00:32:41Z

contracts/contracts/strategies/uniswap/UniswapV3StrategyStorage.sol

+    uint256 public maxTVL; // In USD, 18 decimals
+
+    // Deposits to reserve strategy when contract balance exceeds this amount
+    uint256 public minDepositThreshold0;


I would pack these into a struct with two uint128 numbers so it takes a single storage slot.
They are used together so they can be ready into memory once and then used.

naddison36 · 2023-06-23T04:11:51Z

contracts/contracts/strategies/uniswap/UniswapV3LiquidityManager.sol

+    {
+        if (lowerTick > upperTick)
+            (lowerTick, upperTick) = (upperTick, lowerTick);
+        key = int48(lowerTick) * 2**24; // Shift by 24 bits


a left bit shit operator would be cheaper.

key = int48(lowerTick) << 24;

naddison36 · 2023-06-23T04:14:58Z

contracts/contracts/strategies/uniswap/UniswapV3LiquidityManager.sol

+        if (lowerTick > upperTick)
+            (lowerTick, upperTick) = (upperTick, lowerTick);
+        key = int48(lowerTick) * 2**24; // Shift by 24 bits
+        key = key + upperTick;


a bit or (|) could also be used but that costs the same as an add operation so best to leave the add.

naddison36 · 2023-06-23T04:44:23Z

contracts/contracts/vault/VaultStorage.sol

    event TrusteeFeeBpsChanged(uint256 _basis);
    event TrusteeAddressChanged(address _address);
    event NetOusdMintForStrategyThresholdChanged(uint256 _threshold);
+    event AssetTransferredToUniswapV3Strategy(


Can this have a more general name? It seems very specific to Uniswap V3

naddison36 · 2023-06-23T04:46:13Z

contracts/contracts/vault/VaultStorage.sol

        bool isSupported;
        uint256 _deprecated; // Deprecated storage slot
+        // Set to true if the Strategy is an instance of `UniswapV3Strategy`
+        bool isUniswapV3Strategy;


put before _deprecated so it's in the same storage slot as isSupported

I'd like to get rid of the _deprecated property in the next Vault deploy. I've raise PR #1653 against collateral swaps to change this.

naddison36 · 2023-06-23T04:59:24Z

contracts/contracts/vault/VaultAdmin.sol

+     * @param asset The asset to deposit
+     * @param amount Amount of tokens to deposit
+     */
+    function depositToUniswapV3Reserve(address asset, uint256 amount)


It'd be nice to come up with a more generic name for this. What if we used other AMMs like Curve, Balancer and Maverick to deposit/withdraw from other strategies?

* removed unused StableMath from UniswapV3Strategy * Updated Uniswap V3 Natspec * Added Uniswap V3 strategy diagrams

sparrowDom · 2023-07-06T12:49:54Z

just going through all the open PRs. We are still planning to complete this, but has been on the back-burner for now?

naddison36 · 2023-07-07T02:32:38Z

It's on the back-burner for now

shahthepro · 2023-09-21T16:34:54Z

Closing this PR for now, can always reopen this branch if it becomes a priority again in future

shahthepro and others added 14 commits March 1, 2023 12:24

checkpoint

0cdb508

Merge branch 'master' into shah/uniswap-v3-strategy

ceaeb92

Switch to timelock

2da919c

unblock myself

8fd2464

Merge branch 'master' into shah/uniswap-v3-strategy

84fc866

More updates

5c2d34a

Add some comments

861726d

Document more functions

2bc18cb

Fix bugs

700fd57

Update fork tests

e30b62f

Lint

a08eb2f

Add more fork tests

d4f982c

Linter/prettier

91fd51d

Slither fixes

86523a5

DanielVF added the contracts Works related to contracts label Mar 6, 2023

sparrowDom reviewed Mar 7, 2023

View reviewed changes

DanielVF reviewed Mar 7, 2023

View reviewed changes

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol Outdated Show resolved Hide resolved

DanielVF reviewed Mar 7, 2023

View reviewed changes

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol Outdated Show resolved Hide resolved

DanielVF reviewed Mar 7, 2023

View reviewed changes

contracts/contracts/strategies/GeneralizedUniswapV3Strategy.sol Outdated Show resolved Hide resolved

DanielVF reviewed Mar 7, 2023

View reviewed changes

sparrowDom reviewed Mar 7, 2023

View reviewed changes

shahthepro added 2 commits March 8, 2023 21:04

Updates

cc29fc8

Few more tweaks

bd8d3f9

shahthepro added 4 commits April 21, 2023 14:39

Set Governor address to zero on implementation contracts (#1299)

0e8a804

Uniswap - N14 - Remove unused imports (#1312)

f71b05c

* Remove unused imports * Add back import

Merge branch 'master' into shah/uniswap-v3-strategy

88b341f

shahthepro marked this pull request as ready for review April 21, 2023 13:51

shahthepro requested a review from franckc as a code owner April 21, 2023 13:51

Merge branch 'master' into shah/uniswap-v3-strategy

bc35c9c

shahthepro requested a review from naddison36 as a code owner May 24, 2023 13:58

Update yarn.lock

af9e4a5

naddison36 self-assigned this Jun 1, 2023

naddison36 mentioned this pull request Jun 5, 2023

Review OUSD Uniswap V3 strategy for the launch #1556

Open

shahthepro added 4 commits June 6, 2023 14:50

Merge master

138ad8f

Lint

6c34043

Update deploy script ID

3829c63

Merge branch 'master' into shah/uniswap-v3-strategy

c1faa2b

shahthepro added 7 commits June 7, 2023 09:46

Merge branch 'master' into shah/uniswap-v3-strategy

78103ee

Merge branch 'master' into shah/uniswap-v3-strategy

ef873fa

Fix unit tests

5f2670b

Use Aave Strategy as reserves

f0689e2

Fix fork tests

79da02d

Fix fork tests

0324ac1

Fix address

042a32b

naddison36 reviewed Jun 23, 2023

View reviewed changes

Uniswap v3 strategy updates (#1652)

5d4063d

* removed unused StableMath from UniswapV3Strategy * Updated Uniswap V3 Natspec * Added Uniswap V3 strategy diagrams

shahthepro closed this Sep 21, 2023

shahthepro deleted the shah/uniswap-v3-strategy branch September 21, 2023 16:35

Uniswap V3 Strategy #1258

Uniswap V3 Strategy #1258

Uh oh!

Conversation

shahthepro commented Mar 2, 2023 • edited by sparrowDom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielVF commented Mar 7, 2023

Security goals

Gas usage

Swap

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparrowDom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparrowDom commented Mar 7, 2023

Uh oh!

shahthepro commented Mar 8, 2023

Uh oh!

codecov bot commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

naddison36 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahthepro commented Mar 2, 2023 •

edited by sparrowDom

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading