Maia DAO Ecosystem - Kamil-Chmielewski's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L340-L348 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L373-L374

Vulnerability details

One of the project assumptions is that anyone can call the restakeToken function on someone else's token after the incentive ends (at the start of the new gauge cycle).

File: src/uni-v3-staker/UniswapV3Staker.sol
365:     function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isNotRestake) private {
366:         Deposit storage deposit = deposits[tokenId];
367: 
368:         (uint96 endTime, uint256 stakedDuration) =
369:             IncentiveTime.getEndAndDuration(key.startTime, deposit.stakedTimestamp, block.timestamp);
370: 
371:         address owner = deposit.owner;
372: 
373: @>      // anyone can call restakeToken if the block time is after the end time of the incentive
374: @>      if ((isNotRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();
		
		...

This assumption is broken because everywhere the _unstakeToken is called, the isNotRestake flag is set to true, including the restakeToken function. Because of that, when the caller is not the deposit.owner, the if block will evaluate to true, and the call will revert with NotCalledByOwner() error.

File: src/uni-v3-staker/UniswapV3Staker.sol
340:     function restakeToken(uint256 tokenId) external {
341:         IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
342: @>      if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
343: 
344:         (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
345:             NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);
346: 
347:         _stakeToken(tokenId, pool, tickLower, tickUpper, liquidity);
348:     }

Impact

Lower yield for users, broken 3rd party integration, higher gas usage

The purpose of the restakeToken function is to:

• Enable easier automation - re-staking without the need for manual intervention
• And aggregation - combining multiple actions into a single operation to increase efficiency and reduce transaction costs.

This is also the reason why the UniswapV3Staker contract inherits from Multicallable.

Without the ability to re-stake for someone else, 3rd parties or groups of users won't be able to perform cost and yield efficient batch re-stakes.

As stated in the Liquidity Mining section in the docs - LPs will lose new rewards until they re-stake again. Any delay means fewer rewards -> fewer bHermes utility tokens -> lower impact in the ecosystem. It is very unlikely that users will be able to re-stake exactly at 12:00 UTC every Thursday (to maximise yield) without some automation/aggregation.

Proof of Concept

Since I decided to create a fork test on Arbitrum mainnet, the setup is quite lengthy and is explained in great detail in the following GitHub Gist.

Pre-conditions:

• Alice and Bob are users of the protocol. They both have the 1000 DAI / 1000 USDC UniswapV3 Liquidity position minted.
• The UniswapV3Gauge has weight allocated to it.
• The BaseV2Minter has queued HERMES rewards for the cycle.
• Charlie is a 3rd party that agreed to re-stake Alice's token at the start of the next cycle (current incentive end time)

function testRestake_RestakeIsNotPermissionless() public {
        vm.startPrank(ALICE);
        // 1.a Alice stakes her NFT (at incentive StartTime)
        nonfungiblePositionManager.safeTransferFrom(ALICE, address(uniswapV3Staker), tokenIdAlice);
        vm.stopPrank();

        vm.startPrank(BOB);
        // 1.b Bob stakes his NFT (at incentive StartTime)
        nonfungiblePositionManager.safeTransferFrom(BOB, address(uniswapV3Staker), tokenIdBob);
        vm.stopPrank();

        vm.warp(block.timestamp + 1 weeks); // 2.a Warp to incentive end time
        gauge.newEpoch();                   // 2.b Queue minter rewards for the next cycle

        vm.startPrank(BOB);
        uniswapV3Staker.restakeToken(tokenIdBob); // 3.a Bob can restake his own token
        vm.stopPrank();

        vm.startPrank(CHARLIE);
        vm.expectRevert(bytes4(keccak256("NotCalledByOwner()")));
@>issue uniswapV3Staker.restakeToken(tokenIdAlice); // 3.b Charlie cannot restake Alice's token
        vm.stopPrank();

        uint256 rewardsBob = uniswapV3Staker.rewards(BOB);
        uint256 rewardsAlice = uniswapV3Staker.rewards(ALICE);

        assertNotEq(rewardsBob, 0, "Bob should have rewards");
        assertEq(rewardsAlice, 0, "Alice should not have rewards");

        console.log("=================");
        console.log("Bob's rewards   : %s", rewardsBob);
        console.log("Alice's rewards : %s", rewardsAlice);
        console.log("=================");
    }

When used with multicall as it probably would in a real-life scenario, it won't work either.

Change Charlie's part to:

bytes memory functionCall1 = abi.encodeWithSignature(
	"restakeToken(uint256)",
	tokenIdAlice
);
bytes memory functionCall2 = abi.encodeWithSignature(
	"restakeToken(uint256)",
	tokenIdBob
);

bytes[] memory data = new bytes[](2);
data[0] = functionCall1;
data[1] = functionCall2;

vm.startPrank(CHARLIE);
address(uniswapV3Staker).call(abi.encodeWithSignature("multicall(bytes[])", data));
vm.stopPrank();

Tools Used

Manual Review

Recommended Mitigation Steps

A simple fix is to change the isNotRestake flag inside the restakeToken function to false:

diff --git a/src/uni-v3-staker/UniswapV3Staker.sol b/src/uni-v3-staker/UniswapV3Staker.sol
index 5970379..d7add32 100644
--- a/src/uni-v3-staker/UniswapV3Staker.sol
+++ b/src/uni-v3-staker/UniswapV3Staker.sol
@@ -339,7 +339,7 @@ contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
 
     function restakeToken(uint256 tokenId) external {
         IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
-        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
+        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);
 
         (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
             NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

A more complicated fix, which would reduce code complexity in the future, would be to rename the isNotRestake flag to isRestake.

This way, one level of indirection would be reduced.

diff --git a/src/uni-v3-staker/UniswapV3Staker.sol b/src/uni-v3-staker/UniswapV3Staker.sol
index 5970379..43ff24c 100644
--- a/src/uni-v3-staker/UniswapV3Staker.sol
+++ b/src/uni-v3-staker/UniswapV3Staker.sol
@@ -354,15 +354,15 @@ contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
     /// @inheritdoc IUniswapV3Staker
     function unstakeToken(uint256 tokenId) external {
         IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
-        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
+        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);
     }
 
     /// @inheritdoc IUniswapV3Staker
     function unstakeToken(IncentiveKey memory key, uint256 tokenId) external {
-        _unstakeToken(key, tokenId, true);
+        _unstakeToken(key, tokenId, false);
     }
 
-    function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isNotRestake) private {
+    function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isRestake) private {
         Deposit storage deposit = deposits[tokenId];
 
         (uint96 endTime, uint256 stakedDuration) =
@@ -371,7 +371,7 @@ contract UniswapV3Staker is IUniswapV3Staker, Multicallable {
         address owner = deposit.owner;
 
         // anyone can call restakeToken if the block time is after the end time of the incentive
-        if ((isNotRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();
+        if ((isRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();

Assessed type

Access Control

Maia DAO - QA Report

Summary

[L-01] The restakeToken is missing in the IUniswapV3Staker interface

The external function UniswapV3Staker#restakeToken is missing from the IUniswapV3Staker.sol.

Since the re-staking functionality is supposed to be permissionless, off-chain automation tools will call restakeToken to keep the user's token always staked. It is important that they have frictionless access to it via the interface.

Fix:

diff --git a/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol b/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
index 895c505..f957dd8 100644
--- a/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
+++ b/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
@@ -239,6 +239,14 @@ interface IUniswapV3Staker is IERC721Receiver {
     /// @param tokenId The ID of the token to stake
     function stakeToken(uint256 tokenId) external;
 
+    /*//////////////////////////////////////////////////////////////
+                            RESTAKE TOKEN LOGIC
+    //////////////////////////////////////////////////////////////*/
+    
+    /// @notice Restakes a Uniswap V3 LP token
+    /// @param tokenId The ID of the token to restake
+    function restakeToken(uint256 tokenId) external;
+
     /*//////////////////////////////////////////////////////////////
                         GAUGE UPDATE LOGIC
     //////////////////////////////////////////////////////////////*/

[L-02] The functionality of decrementGaugeBoost and decrementGaugesBoostIndexed differs

According to the spec in the IERC20Boost interface, the decrementGaugeBoost and decrementGaugesBoostIndexed should function the same, with the latter being an indexed version of the former.

It is not the case. The difference lies in the way they handle deprecated gauges. The decrementGaugesBoostIndexed function, when supplied with a deprecated gauge, will remove the user gauge boost entirely. The decrementGaugeBoost will either decrease the deprecated gauge's user boost or delete it, depending on whether the uint256 boost is >= gaugeState.userGaugeBoost.

This is happening because decrementGaugesBoostIndexed explicitly checks if _deprecatedGauges.contains(gauge) while decrementGaugeBoost does not.

It is up to the protocol team to decide whether the user gauge boost should be removed from deprecated gauges or just be decremented.

Things to consider:

• Users expect consistent behaviour from similar functions.
• Since gauges can be reactivated, someone might not want to remove the boost from it.

If boost were to be removed:

diff --git a/src/erc-20/ERC20Boost.sol b/src/erc-20/ERC20Boost.sol
index a1da4df..9f13074 100644
--- a/src/erc-20/ERC20Boost.sol
+++ b/src/erc-20/ERC20Boost.sol
@@ -174,7 +174,7 @@ abstract contract ERC20Boost is ERC20, Ownable, IERC20Boost {
     /// @inheritdoc IERC20Boost
     function decrementGaugeBoost(address gauge, uint256 boost) public {
         GaugeState storage gaugeState = getUserGaugeBoost[msg.sender][gauge];
-        if (boost >= gaugeState.userGaugeBoost) {
+        if (_deprecatedGauges.contains(gauge) || boost >= gaugeState.userGaugeBoost) {
             _userGauges[msg.sender].remove(gauge);
             delete getUserGaugeBoost[msg.sender][gauge];

And if it were to be decremented:

diff --git a/src/erc-20/ERC20Boost.sol b/src/erc-20/ERC20Boost.sol
index a1da4df..51bad76 100644
--- a/src/erc-20/ERC20Boost.sol
+++ b/src/erc-20/ERC20Boost.sol
@@ -209,7 +209,7 @@ abstract contract ERC20Boost is ERC20, Ownable, IERC20Boost {
 
             GaugeState storage gaugeState = getUserGaugeBoost[msg.sender][gauge];
 
-            if (_deprecatedGauges.contains(gauge) || boost >= gaugeState.userGaugeBoost) {
+            if (boost >= gaugeState.userGaugeBoost) {
                 require(_userGauges[msg.sender].remove(gauge)); // Remove from set. Should never fail.
                 delete getUserGaugeBoost[msg.sender][gauge];

[L-03] Incorrect NatSpec comment

IUniswapV3Staker#claimAllRewards

The @notice comment on the IUniswapV3Staker#claimAllRewards function is incorrect. It states that the claimAllRewards function transfers amountRequested to the recipient, while it transfers all of the rewards.

It also benefits the users to add the information that the unstakeToken or restakeToken should be called before this function. Otherwise, the reward balance won't be updated.

Fix:

diff --git a/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol b/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
index 895c505..6b57bed 100644
--- a/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
+++ b/src/uni-v3-staker/interfaces/IUniswapV3Staker.sol
@@ -204,7 +204,7 @@ interface IUniswapV3Staker is IERC721Receiver {
     /// @return reward The amount of reward tokens claimed
     function claimReward(address to, uint256 amountRequested) external returns (uint256 reward);
 
-    /// @notice Transfers `amountRequested` of accrued `rewardToken` rewards from the contract to the recipient `to`
+    /// @notice Transfers all of the accrued `rewardToken` rewards from the contract to the recipient `to`
     /// @param to The address where claimed rewards will be sent to
     /// @return reward The amount of reward tokens claimed
     function claimAllRewards(address to) external returns (uint256 reward);

IERC20Boost#numDeprecatedGauges

The @notice comment on the numDeprecatedGauges function is incorrect. This function returns the number of deprecated gauges, not live gauges.

Fix:

diff --git a/src/erc-20/interfaces/IERC20Boost.sol b/src/erc-20/interfaces/IERC20Boost.sol
index 7dfe65c..1ecbdec 100644
--- a/src/erc-20/interfaces/IERC20Boost.sol
+++ b/src/erc-20/interfaces/IERC20Boost.sol
@@ -97,7 +97,7 @@ interface IERC20Boost {
     function deprecatedGauges() external view returns (address[] memory);
 
     /**
-     * @notice returns the number of live gauges
+     * @notice returns the number of deprecated gauges
      */
     function numDeprecatedGauges() external view returns (uint256);

IERC20Boost#freeGaugeBoost

The @notice comment on the freeGaugeBoost function is incorrect. It was copied over from the userGauges function.

The function returns the amount of boost that the user has left to allocate.

Fix:

diff --git a/src/erc-20/interfaces/IERC20Boost.sol b/src/erc-20/interfaces/IERC20Boost.sol
index 7dfe65c..3aee801 100644
--- a/src/erc-20/interfaces/IERC20Boost.sol
+++ b/src/erc-20/interfaces/IERC20Boost.sol
@@ -102,7 +102,7 @@ interface IERC20Boost {
     function numDeprecatedGauges() external view returns (uint256);
 
     /**
-     * @notice returns the set of gauges the user has allocated to, they may be live or deprecated.
+     * @notice returns the amount of boost that the user has left to allocate to gauges.
      */
     function freeGaugeBoost(address user) external view returns (uint256);

IFlywheelGaugeRewards#queueRewardsForCyclePaginated

The @notice comment on the queueRewardsForCyclePaginated function is incorrect. It was copied over from the queueRewardsForCycle function.

It does not iterate over all live gauges but on the number of gauges specified in the numRewards parameter.

Fix:

diff --git a/src/rewards/interfaces/IFlywheelGaugeRewards.sol b/src/rewards/interfaces/IFlywheelGaugeRewards.sol
index e1a9137..2618557 100644
--- a/src/rewards/interfaces/IFlywheelGaugeRewards.sol
+++ b/src/rewards/interfaces/IFlywheelGaugeRewards.sol
@@ -71,7 +71,7 @@ interface IFlywheelGaugeRewards {
      */
     function queueRewardsForCycle() external returns (uint256 totalQueuedForCycle);
 
-    /// @notice Iterates over all live gauges and queues up the rewards for the cycle
+    /// @notice Iterates over a number of gauges specified by `numRewards` and queues up the rewards for the cycle
     function queueRewardsForCyclePaginated(uint256 numRewards) external;

[N-01] userDelegatedVotes does not belong to admin operations

Code styling

The userDelegatedVotes function is placed next to the admin operations in the IERC20MultiVotes interface. It is a view function that should be placed a couple of lines below, next to other "Delegation Logic" functions.

IERC20MultiVotes#userDelegatedVotes

Fix:

diff --git a/src/erc-20/interfaces/IERC20MultiVotes.sol b/src/erc-20/interfaces/IERC20MultiVotes.sol
index c628293..476c9aa 100644
--- a/src/erc-20/interfaces/IERC20MultiVotes.sol
+++ b/src/erc-20/interfaces/IERC20MultiVotes.sol
@@ -85,15 +85,15 @@ interface IERC20MultiVotes {
      */
     function setContractExceedMaxDelegates(address account, bool canExceedMax) external;
 
+    /*///////////////////////////////////////////////////////////////
+                        DELEGATION LOGIC
+    //////////////////////////////////////////////////////////////*/
+    
     /**
      * @notice mapping from a delegator to the total number of delegated votes.
      */
     function userDelegatedVotes(address) external view returns (uint256);
 
-    /*///////////////////////////////////////////////////////////////
-                        DELEGATION LOGIC
-    //////////////////////////////////////////////////////////////*/
-
     /**
      * @notice Get the amount of votes currently delegated by `delegator` to `delegatee`.
      * @param delegator the account which is delegating votes to `delegatee`.

[N-02] The return value of EnumerableSet.remove is unchecked

The EnumerableSet.remove function returns a status boolean on a successful removal. The ERC20Boost#decrementGaugeBoost function does not check for that value. Other similar functions like decrementGaugeAllBoost, decrementGaugesBoostIndexed and decrementAllGaugesAllBoost do check the return value.

In a situation where incorrect gauge address were passed to the decrementGaugeBoost function, the Detach event would be emitted nonetheless.

Fix:

diff --git a/src/erc-20/ERC20Boost.sol b/src/erc-20/ERC20Boost.sol
index a1da4df..f12fd2c 100644
--- a/src/erc-20/ERC20Boost.sol
+++ b/src/erc-20/ERC20Boost.sol
@@ -175,7 +175,7 @@ abstract contract ERC20Boost is ERC20, Ownable, IERC20Boost {
     function decrementGaugeBoost(address gauge, uint256 boost) public {
         GaugeState storage gaugeState = getUserGaugeBoost[msg.sender][gauge];
         if (boost >= gaugeState.userGaugeBoost) {
-            _userGauges[msg.sender].remove(gauge);
+            require(_userGauges[msg.sender].remove(gauge));
             delete getUserGaugeBoost[msg.sender][gauge];
 
             emit Detach(msg.sender, gauge);

[N-03] Missing NatSpec on queueRewardsForCyclePaginated

The NatSpec @param numRewards is missing in the function IFlywheelGaugeRewards#queueRewardsForCyclePaginated

Fix:

diff --git a/src/rewards/interfaces/IFlywheelGaugeRewards.sol b/src/rewards/interfaces/IFlywheelGaugeRewards.sol
index e1a9137..657fbea 100644
--- a/src/rewards/interfaces/IFlywheelGaugeRewards.sol
+++ b/src/rewards/interfaces/IFlywheelGaugeRewards.sol
@@ -71,7 +71,10 @@ interface IFlywheelGaugeRewards {
      */
     function queueRewardsForCycle() external returns (uint256 totalQueuedForCycle);
 
-    /// @notice Iterates over all live gauges and queues up the rewards for the cycle
+    /**
+     * @notice Iterates over all live gauges and queues up the rewards for the cycle
+     * @param numRewards the number of gauges to queue rewards for
+     */
     function queueRewardsForCyclePaginated(uint256 numRewards) external;

[N-04] Misleading function name

The BaseV2Minter#getRewards function has a misleading function name. Usually, the functions with get and set prefixes are used for getters and setters (see Admin setters in the same contract).

The getRewards function is by no means a simple getter since it handles the transfer of HERMES tokens from the BaseV2Minter for weekly rewards.

Consider changing the naming convention to describe code intentions better.

[N-05] Incorrect comment

The comment on the internal function ERC20Boost#_removeGauge is incorrect. It states that the removal of the gauge should fail if the gauge were not present in the _deprecatedGauges set.

The correct behaviour would be: the removal should fail if the gauge were already present in the _deprecatedGauges set.

Fix:

diff --git a/src/erc-20/ERC20Boost.sol b/src/erc-20/ERC20Boost.sol
index a1da4df..a0f808f 100644
--- a/src/erc-20/ERC20Boost.sol
+++ b/src/erc-20/ERC20Boost.sol
@@ -275,7 +275,7 @@ abstract contract ERC20Boost is ERC20, Ownable, IERC20Boost {
     }
 
     function _removeGauge(address gauge) internal {
-        // add to deprecated and fail loud if not present
+        // add to deprecated and fail loud if already present
         if (!_deprecatedGauges.add(gauge)) revert InvalidGauge();
 
         emit RemoveGauge(gauge);

[N-06] Typo in the comment

There is a typo in the word rewards in the FlywheelGaugeRewards#queueRewardsForCycle function.

Fix:

-        /// @dev Update minter cycle and queue rewars if needed.
+        /// @dev Update minter cycle and queue rewards if needed.