uniswap / v3-staker

Canonical liquidity mining contract for Uniswap V3

uniswap-v3-staker

This is the canonical staking contract designed for Uniswap V3.

Deployments

Note that the v1.0.0 release is susceptible to a high-difficulty, never-exploited vulnerability. For this reason, please use the v1.0.2 release, deployed and verified on Etherscan:

Network	Explorer
Mainnet	https://etherscan.io/address/0xe34139463bA50bD61336E0c446Bd8C0867c6fE65
Arbitrum One	https://arbiscan.io/address/0xe34139463bA50bD61336E0c446Bd8C0867c6fE65
Optimism	https://optimistic.etherscan.io/address/0xe34139463bA50bD61336E0c446Bd8C0867c6fE65
Base	https://basescan.org/address/0x42be4d6527829fefa1493e1fb9f3676d2425c3c1

⚠️DEPRECATED⚠️: For historical verification purposes only, the staker at tag v1.0.0 was deployed at the address: 0x1f98407aaB862CdDeF78Ed252D6f557aA5b0f00d

Links

• Contract Design

Development and Testing

yarn
yarn test

Gas Snapshots

# if gas snapshots need to be updated
$ UPDATE_SNAPSHOT=1 yarn test

Contract Sizing

yarn size-contracts

v3-staker's Issues

Use block.timestamp

Remove all references to BlockTimestamp.sol and rely on block.timestamp
instaed.

OnERC721Received hook

Should the staker automatically stake any ERC721 it receives through safeTransferFrom?

Only cost I can think of is if this makes the explicit stake function or other ways of staking more difficult, but maybe those could use transferFrom instead?

Ability to view available rewards without unstaking or claiming

A user should be able to view how many rewards they have for each reward token without claiming their tokens or unstaking their position. This should make it easier fr the user to decide whether it's worth the cost to claim right now.

We'd need a view function that calculates the rewards that are available from the sum of positions that have been unstaked and positions that are currently being staked.

unstakeToken Accounting

Ensure all the numbers tie out correctly when someone calls unstakeToken.

To Do

• @danrobinson will be providing some example scenarios to test.
• Write script and start testing scenarios

Notes for @omarish (sorry if cryptic)

• Advanced test cases
• what to do if the token amount is a uint256? Should fail.
• Double-check that all writes reference the actual object (Incentive memory incentive = incentives[incentiveId];)

State not updated when token is unstaked

The State struct representing the users position is not updated when the token is unstaked in unstakeToken. Results of this mean unstakeToken being called more than once and deposits[tokenId] potentially being negative.

It also stops the user from being able to restake the token because we require that the stakes[params.tokenId][incentiveId].exists != true in _stakeToken.

Consider `onERC721Received` hook taking `data` for arbitrary calls rather than limiting to stake

multiple staking is really the only thing I can imagine someone would want to do when transferring (no reason to withdraw claimrewards or unstake

Integration Tests should have explicit actors

See #23 for context.

It's important that the contract calls in integration test suite are explicit - i.e instead of saying ERC20.deploy(12345), we should say something like ERC20.connect(erc20Owner).deploy(12345), or ERC20.deploy(12345, erc20Owner).

To Do

• Add actors module
• Specify the actor everywhere a contract gets deployed/called

Accounting Integration Tests

We're done with individual unit testing for the seven main contract calls. Those tests covered isolated functionality.

Now, we need to add some integration tests. What do I mean by that? Here are some scenarios:

• Alice create a pool for TOK1, TOK2 and adds liquidity to it within a certain range.
• There's an incentive program created that distributes in TOK3
• Some trades happen, all within Alice's bounds.
• Alice wants to withdraw, make sure she has all the fees
• Alice tries to withdraw again, make sure she can't double-withdraw
• The protocol fee gets turned on - make sure the amounts still tie out
• Bob comes in and adds liquidity that's partially within range
• Some trades happen
• Make sure that the incentives are split correctly between the two LPs

Can we remove rewardToken from `Incentive` struct?

Delete rewardToken from Incentive struct (and replace incentive.rewardToken with param.rewardToken where it appears)

Ability to claim rewards without unstaking?

Would be nice as a farmer to be able to harvest rewards periodically but keep the farm going.
Seems like this would be possible by updating secondsPerLiquidityInitialX128 when claiming?

Idea: Merge CreateIncentiveParams and EndIncentiveParams

CreateIncentiveParams and EndIncentiveParams are pretty similar:

struct CreateIncentiveParams {
    address pool;
    address rewardToken;
    uint128 totalReward;
    uint32 startTime;
    uint32 endTime;
    uint32 claimDeadline;
}

struct EndIncentiveParams {
    address pool;
    address rewardToken;
    uint32 claimDeadline;
    uint32 endTime;
    uint32 startTime;
}

I wonder how much space we'd save if we used the same struct for both of these, and passed totalReward as a separate argument to createIncentive. So the function signature would become function createIncentive(CreateIncentiveParams memory params, uint128 totalReward).

Adjust timestamps to be either uint64 or uint256

Forgot where we landed if someone could clarify, but update timestamp parameters to appropriate uint bits.

feature request: allow data to be specific in the `nft#safeTransferFrom` of `withdrawToken`

could be useful, e.g. to unstake and decrease liquidity in one tx

Allow Stakers to Unstake after an incentive has been ended by the creator

Currently, if a creator calls endIncentive the incentive gets deleted. However, unstake confirms you're unstaking against an existing incentive. Hence if you try to unstake after the incentive has been deleted, you will get reverted and your NFT is held hostage.

Solution 1
In unstakeToken, do not check that incentive exists but check that stake exists (which can only be created against an existing incentive).
Create flow for non-existent, 0 rewardRate Incentive

Solution 2
Instead of deleting incentive: set totalRewardUnclaimed to 0.
In unstakeToken check that claimDeadline has not yet passed

Waffle MockProvider that persists fixtures between test suite runs

In doing the accounting integration tests, my loop is:

1. Run a fixture
2. Run a single test that fails
3. Fix the code
4. Re-run the test suite

Fixtures are great when you're running multiple tests (i.e multiple it blocks with a single fixture). Is there a way to use waffle's MockProvider so that it persists chain state between steps 2 and 4 above?

One answer would be to run ganache-cli and connect to that, but I'd like to keep all the great debugging features I get through hardhat.

This should be mostly straightforward to implement. I looked into it and I think the way to do it is by subclassing waffle.MockProvider. And it would be okay if we had to manually call .snapshot() to explicitly snapshot and save chain state.

rewards stored value should be uint256 instead of uint128

mapping(address => mapping(address => uint128)) public rewards;

v0.1 Burndown List

Misc todos before v0.1 is ready for security review:

High Priority

• Just making sure the checks on startTime, endTime, claimDeadline are all working properly. I.e are they consistently using >= or >?
• Feeling a little uneasy that this effect is so high up
• Update design doc based on the latest hackmd: https://hackmd.io/LnZax7TkR6yRUCoSlmRGoA?edit
• Do we want to rename claimDeadline to rewardRevocableAt? See this discussion
• Does UniV3 ever plan on having multiple NonfungiblePositionManagers? I think the answer is no, but double-checking.
• Make sure everything important is evented

Test Scenarios

• More tests around claiming rewards
• Test: multiple incentives on the same rewardToken. Make sure claimReward functions properly
• Are we testing any fee amounts other than FeeAmount.MEDIUM, and if not, should we be?

Disallow staking with 0 liquidity

Also:

• all stake truthiness checks can check that liquidity != 0
• delete exists bool from Stake struct

Consider not deleting `Incentive` on endIncentive

If Incentive is not deleted, then:
endIncentive + _stakeToken must check totalRewardUnclaimed > 0 for valid incentives check rather than rewardToken value which will remain non-zero.

pros to deleting: tx is cheaper atm bc of storage clears.

cons: (plz fill this in someone I'm blanking)

Should liquidity be on Deposit or Stake?

Should liquidity be on Deposit or Stake?

If on Stake, that adds an SSTORE to every stake. If it's on Stake, it makes it slightly easier to top up a deposit without having to withdraw it, plus maybe no need for exists flag.

createIncentive should require `startTime` has not passed

Cleanup: Decide on consistent revert message format

Complex Staking Scenario Tests

Integration tests for complex staking scenarios:

• Multiple people staking, some within range, some not within range.
• They all try to withdraw; make sure that the balances tie out properly.

Core Events Must Emit Events

The six main functions must emit events:

• TokenDeposited
• TokenWithdrawn
• IncentiveCreated
• IncentiveEnded
• TokenStaked
• TokenUnstaked

The codebase is still changing quickly so I'm not sure the parameters yet. Happy
to describe this in more detail if someone is interested in running with this.

Also, there must be test coverage around these.

UniswapV3Staker Natspec Comments

UniswapV3Staker should have natspec comments throughout.

Cleanup: Get full natspec documentation going

Ideally every function/struct has complete natspec documentation. Like this: https://github.com/omarish/uniswap-v3-staker/blob/master/contracts/interfaces/IUniswapV3Staker.sol#L27-L34

all functions have @notice @param @return documentation

For structs use named parameter constructor

Make the transfer in unstake separate

If someone creates an incentive token that can be frozen, paused, etc., we do not want that to prevent stakers from withdrawing their liquidity if they cannot transfer on this line
https://github.com/omarish/uniswap-v3-staker/blob/8bd8a98b9bdf9d65fa5ab72acc66ef2a35337e4e/contracts/UniswapV3Staker.sol#L295-L298

The solution would be to make 'collecting' the reward tokens and unstaking separate methods

In Depth Math review of rewards calculations

Math can be found in _getRewardAmount()

Includes a full check for overflows throughout the staker contract

allow multicall

User should be able to batch operations together, such as depositing a token and then staking with multiple incentives, or unstaking with multiple incentives, collecting tokens and withdrawing

We use the multicall pattern in periphery for this

Refactor getIncentiveId visibility

Staking Calculations Ledger

@danrobinson to send over a short doc that walks through the math for staking scenarios. We discussed this 1:1; this issue is just a placeholder for the work.

Happy to describe this further if others want to get involved.

Consider removing `depositToken` function

onERC721Received hook covers all current usecases.

Identify Incentive in Events where incentive matters

We do a lot of Stake Unstake and now a ClaimRewardFromCurrentStake and the incentive is an important component in these actions. The events should probably signal which is incentive is tied to these actions. Can we include the bytes32 id or do we need to include all parameters to reach the bytes32 id?

I'm leaning towards bytes32 because it's cleaner and interfaces will probably need to cache the id anyway, especially if they're using the public mappings at all....... 🤔

Abilty to see rewards earned

I think it would be helpful if users could see how much rewards they have
earned, without unstaking or claiming.

Use TransferHelper safe transfer functions for ERC20

Will verify successful transfers even when certain ERC20's do not return bool
https://github.com/Uniswap/uniswap-v3-periphery/blob/main/contracts/libraries/TransferHelper.sol
can import from periphery

endIncentive: get creator from calldata to allow anyone to endIncentive

Rewards will still be sent to incentiveCreator

Stake & Unstake Functionality

The staking contract should support:

• stake() to stake an NFT
• unstake() to calculate accrued time and distribute rewards
• Full unit and integration testing for both of these features.

why is endIncentive nonReentrant?

https://github.com/omarish/uniswap-v3-staker/blob/8bd8a98b9bdf9d65fa5ab72acc66ef2a35337e4e/contracts/UniswapV3Staker.sol#L110

I don't see why it should be

createIncentive: transfer should happen after state changes

plus do a sanity check over contract for other state change / transfer ordering

Test coverage for core functionality

Test Cases

Test cases are documented in test/UniswapV3Staker.spec.ts.

consider allowing msg.sender to specify a return address for rewards to be refunded to

creator would become something like beneficiary or returnee

What's the point of numberOfStakes ?

I'm glad to see this project moving forward, but as I'm going through the code, I'm having trouble understanding "numberOfStakes".

In fact, nothing prevents a user from calling several times the external function stakeToken(), which will increment numberOfStakes and override the value for stakes[params.tokenId][incentiveId].

stakes[params.tokenId][incentiveId] = Stake(
    secondsPerLiquidityInsideX128
)

The user, can have a numberOfStakes > 1 for his deposit, but only one snapshot of seconds per liquidity for the deposit position range is stored. If numberOfStakes > 1, you need to call unstakeToken x times to decrement numberOfStakes and be able to call withdrawToken() => require(deposit.numberOfStakes == 0, 'NUMBER_OF_STAKES_NOT_ZERO');.

Why is it not a Boolean called "staked" instead ? And check if deposit already staked before unstaking ?

There are certainly elements that I am missing, and my comment may not be relevant. But I will be happy to understand this mechanism a little better :)

Time travel in integration tests

Deploy Staking Contract to Testnet

Do test deploy to all ethereum testnets once security audit is complete.

Update the docs in this repository to include testnet contract addresses.

Uniswap typechain artifacts building as yarn task

Heyo,

(slight background, working on the v3 rewards for rally.io so very much invested in this project/getting this to work in some way)

Have you seen how https://github.com/Uniswap/uniswap-interface does the type building? https://github.com/Uniswap/uniswap-interface/blob/main/package.json#L120-L122 is an alternative that doesn't muck with submodules. I ended up using it for a quick pool analysis tool I'm working on, I liked it.

Thoughts? Happy to throw up a PR if there is interest in migrating to this method.

claimRewards: specify an amount to claim

allows someone to claim partial rewards

Gas snapshots are inconsistent

I'm getting different gas snapshots on my localhost and on CI. Is this happening to anyone else?

Refactor Unit Test Fixtures

The unit test fixtures have a lot of repeated setup code. For example, see all the places we're setting up the environment:

• https://github.com/omarish/uniswap-v3-staker/blob/master/test/UniswapV3Staker.unit.spec.ts#L172
• https://github.com/omarish/uniswap-v3-staker/blob/master/test/UniswapV3Staker.unit.spec.ts#L393

It'd be nice if we had a single place to set this up. There are a couple important things to check for while doing this refactor:

1. That we're testing properly and delineating between the tokens: in some cases we're testing with tokens[1] and tokens[2] like this, and we should make sure that all the test cases are working when there are three distinct tokens: the two for the Uniswap pair, and a third for the reward token.
2. Actors: important to verify that the unit tests are assuming the right identities. Example: make sure that the account receiving the staking rewards is different from the account creating the incentive, etc. See this as an idea of how we could implement this.

@nickblt made a great point that we're doing a lot of our fixture loading in beforeEach blocks; that's probably what's causing the re-entrancy calls in the accounting integration tests (see #30).

For now, we're going to split this up into two separate PRs:

1. One PR to refactor fixtures in the unit tests, with the goal of being able to utilize how hardhat does snapshotting. @nickblt is taking lead on this.
2. Another PR to refactor how we're keeping track of actors and who is calling contracts. See #34.

Increase of liquidity while staker holds nft is possible and should be accounted for

This opens up the staker contract to a flash loan vulnerability that could drain the reward pool.

Steps in english:

1. Create an NFT with a small position that will stay inside the tick range
2. Stake that NFT
3. Allow time to pass to accumulate seconds inside
4. In a single transaction flash loan, increase liquidity with loan, unstake with very large liquidity position, pay back loan
5. profit in reward tokens

Research context:

NonfungiblePositionManager does not require owner for increase in liquidity: https://github.com/Uniswap/uniswap-v3-periphery/blob/a0e0e5817528f0b810583c04feea17b696a16755/contracts/NonfungiblePositionManager.sol#L198

staker only checks liquidity on unstake: https://github.com/omarish/uniswap-v3-staker/blob/4eab18175aaf8630b8ab19a1a6d99337d0e6435b/contracts/UniswapV3Staker.sol#L205-L221

Proof of concept accounting test: https://gist.github.com/nickblt/d3ace635e1467b4193783b06c2d89760#file-flash-vuln-ts-L139