User forfeits accumulated, unclaimed UniV3 fees upon staking with UniswapV3Staker

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L376-L390

Vulnerability details

As is stated in the Maia docs, staked positions in UniswapV3Staker "will give up any fees until they unstake". However, the docs do not mention that any accumulated fees earned by the position prior to staking are also forfeited to UniswapV3Staker. This results in a direct loss of user funds. Given the misleading statement in the docs, I would argue that this is not a simple case of user error.

Impact

When calling UniswapV3Staker#unstake, the contract calls NonFungiblePositionManager#collect, which collects the fees earned up to a maximum amount, and sends them to the recipient (Uniswap docs).

File: src/uni-v3-staker/UniswapV3Staker.sol

        {
            // scope for bribeAddress, avoids stack too deep errors
            address bribeAddress = bribeDepots[key.pool];

            if (bribeAddress != address(0)) {
                nonfungiblePositionManager.collect( // @audit collects fees earned from Uniswap and sends them to bribeAddress
                    INonfungiblePositionManager.CollectParams({
                        tokenId: tokenId,
                        recipient: bribeAddress,
                        amount0Max: type(uint128).max,
                        amount1Max: type(uint128).max
                    })
                );
            }
        }

In this case, type(uint128).max is passed meaning that all available fees will be collected and sent to bribeAddress. This indeed includes any fees that were earned but not collected prior to staking.

Due to this issue, users who interact with the UniswapV3Staker will give up more Uniswap fees than they expect, which will accumulate in the bribeAddress at the time of unstaking. This can either be a small amount, if the user mints the position NFT using NonFungiblePositionManager and then quickly sends it to UniswapV3Staker, or a large amount, if the user held the position for a long time before sending it to UniswapV3Staker. This value leak will affect every user who wishes to initiate a stake.

Proof of Concept

Consider the following scenario:

• Alice mints a position using Uniswap's NonFungiblePositionManager
• Months pass and Alice's position earns considerable rewards from fees
• Alice decides to send the NFT to UniswapV3Staker to stake it
• Alice unstakes, and all of the fees earned from Uniswap since the positions inception are sent to bribeAddress, rather than just the fees earned during the stake
• Alice's funds have been inadvertently stolen and sent to bribeAddress

Tools Used

Manual review

Recommended Mitigation Steps

Either communicate clearly to the user that they ought to call NonFungiblePositionManager#collect on their NFT before staking it, or add a call to collect in UniswapV3Staker#onERC721Received and refund the fees collected to the caller before staking the NFT. Note that the latter approach would likely be gas intensive, so the tradeoff should be evaluated accordingly.

Assessed type

Uniswap

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c