UniStaker Infrastructure - Al-Qa-qa's results

Summary

---

[L-01] UniStaker::notifyRewardAmount reward transferred check can get down permanently

Unistaker::notifyRewardAmount is designed to be called once the award is distributed, but this function design can not guarantee that the rewards are distributed.

Trail Of Bits has mentioned in their report that users' unclaimed tokens are not checked with it, so the notifyRewardAmount can be fired without distributing the exact amount.

What we want to point out here is that this check can get permanently off (get passed all the time), if the contract receives donations.

The check is done to see that the amount the contract received + remaining rewards are not greater than the actual contract balance.

UniStaker.sol#L594-L596

  function notifyRewardAmount(uint256 _amount) external {
    ...

    if (
      (scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR)
    ) revert UniStaker__InsufficientRewardBalance();

    ...
  }

If the contract receives a donation, let's say 1 WETH. An extra 1 WETH will be considered, whenever a reward is sent, and notifyRewardAmount is fired.

So the _amount value can be atmost 1 WETH less than the actual amount sent by the notifier (if there are no remaining rewards for example).

This issue differs from the case Trail Of Bits mentions, as the problem we mentioned is not because of unclaimed rewards by the users, but because of donations the contract received.

UniStaker devs mentioned that the notifier is required to send _amount before calling notify, so we preferred to make it a LOW issue.

Recommendations

Mitigating this may be a little hard, as we will need to track the amount sent by notifiers, compare the real balance with the amount sent by notifiers, and take suitable actions. And tracking the number of tokens transferred is not an easy task, and will require some changes to UniStaker.

As Trail Of Bits said in their report, mitigating the issue will be hard, and UniStaker devs decided to document the check. So I provide adding in the comment that donations can get the check permanently off.

---

[L-02] MEV searcher will not claim the fees for the swaps that occurred after initializing claimFees function

In the implementation of V3FactoryOwner::claimFee the caller must provide the amount of tokens (first and second pairs) he wants to withdraw, and there is a check that the amount taken is not less than the amount requested by the user

V3FactoryOwner.sol#L189-L195

  function claimFees(... , uint128 _amount0Requested, uint128 _amount1Requested) external returns (uint128, uint128) {
    ...

    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);

    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
    // @audit check that the amount we received is not less than that we requested
    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }
    ...
  }

So the MEV will not be able to extract the maximum profit from the pool (all fees), if there are some swaps or flash loans done at the time the MEV searcher function was in the mempool.

• Let's say the swap fee is 1 token for each pair
• Fees now are (100,100)
• MEV searcher fired claimFee with amounts requested (100,100)
• Some swaps were in the mempool and occurred before claimFee and the total fees are (105,105) now
• MEV searcher will only claim 100 from each pair

If the amount requested by the called of the claimFee is greater than the protocol fees, the amount gets downed to the maximum in uniswapv3-pools

UniswapV3Pool.sol#L853-L854

    function collectProtocol(address recipient, uint128 amount0Requested, uint128 amount1Requested) ... ( ... ) {
        // @audit make the amount equals the fees collected if the amount requested is greater than collected
        amount0 = amount0Requested > protocolFees.token0 ? protocolFees.token0 : amount0Requested;
        amount1 = amount1Requested > protocolFees.token1 ? protocolFees.token1 : amount1Requested;

        ...
    }

So the amount can be set to a big value by the MEV searcher to guarantee the maximum profit, but because of the V3OwnerFactory::claimFees check, the amount received will be < requested and the function will get reverted.

Recommendations

We can make a mechanism similar to the minAmountOut which prevents sandwich attacks.

The caller will provide an amount to request for claiming, and a minimum amount to receive parameters, and if the received amount is < min, the function gets reverted.

Here is a sample of how this can be implemented

V3FactoryOwner::claimFee()

  function claimFees(
    IUniswapV3PoolOwnerActions _pool,
    address _recipient,
    uint128 _amount0Requested,
    uint128 _amount1Requested,
+   uint128 _amount0Min,
+   uint128 _amount1Min
  ) external returns (uint128, uint128) {
    PAYOUT_TOKEN.safeTransferFrom(msg.sender, address(REWARD_RECEIVER), payoutAmount);
    REWARD_RECEIVER.notifyRewardAmount(payoutAmount);
    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);

    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
-   if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
+   if (_amount0 < _amount0Min || _amount1 < _amount1Min) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }
    emit FeesClaimed(address(_pool), msg.sender, _recipient, _amount0, _amount1);
    return (_amount0, _amount1);
  }

So the caller (MEV searcher) can call the function providing the requested amounts with type(uint128).max for example, and set the minimum to the current fees the protocol collects, or the amount that makes him gain a profit.

---

[L-03] Signatures do not have a deadline, even the significant ones

In UniStaker.sol, all functions like (stake and withdraw) can get fired on behalf of the user, where the user (deposited) signs the message and either a 3rth party fires it or he fires it himself.

Functions like (stake, stakeMore, and claim), which transfers funds, do not have a deadline parameter, All functions do not have a deadline parameter, but we want to point to the critical ones.

We can see in permit for example, that the function has a deadline, as it makes the user allow another party to spend his token, and the case is the same for staking, withdrawing, etc..., So having a deadline parameter in the signature, and a deadline check is useful in this case.

Recommendations

Provide a deadline parameter for signatures of the critical functions like stake*() and stakeMore*(), and we can check this deadline in UniStaker::_revertIfSignatureIsNotValidNow() function.

UniStaker::_revertIfSignatureIsNotValidNow

  function _revertIfSignatureIsNotValidNow(
    address _signer,
    bytes32 _hash,
    bytes memory _signature,
+   uint256 deadline
  )
    internal
    view
  {
    bool _isValid = SignatureChecker.isValidSignatureNow(_signer, _hash, _signature);
    if (!_isValid) revert UniStaker__InvalidSignature();

+    // deadline will be `type(uint256).max` for the functions that do not have deadline, or want to allow signature forever
+    if (deadline != type(uint256).max) {
+      require(block.timestamp <= deadline, "UniStaker: Signature expired");
+    }
  }

---

[L-04] Stake More signature do not check beneficiary address changing

In UniStaker contract, STAKE_MORE_TYPEHASH does not contain the beneficiary address in consideration. So the beneficiary address can get changed after signing the message, and this will end up adding funds to the new beneficiary address, and not the one that existed when signing the message.

src/UniStaker.sol#L106-L107

  bytes32 public constant STAKE_MORE_TYPEHASH = // @audit beneficiary is not passed in the signature
    keccak256("StakeMore(uint256 depositId,uint256 amount,address depositor,uint256 nonce)");

• Let's say a deposit has a beneficiary address 0x01
• Message signed by the deposit owner, for giving 0x01 the ability to stake
• depositor changed the beneficial address to 0x02, to let him earn yields too
• The first beneficiary 0x01 fired stakeMoreOnBehalf with his signature signature
• Money goes to the 0x02 instead of 0x01

Since the deposit owner is the one who can make the signatures, and the one who will alter the beneficiary, the problem is not critical, and the severity of it is low. But maybe for further protocol integrations, this can happen who knows?

Recommendations

Add beneficiary parameter to STAKE_MORE_TYPEHASH signature, and provide it with the signature, so that if the beneficiary changes, the message will be invalid

---

---

---

[NC-01] FactoryOwner cannot activate more than one pool at a time

In V3FactoryOwner::setFeeProtocol, the function accepts only one pool to activate the fees on it, and the function is restricted to admins.

Since it's planned to make Uni Governance contract the admin, adding a new pool, will need to make proposal, then vote, then agree, and execute in the last which is not a straight thing.

New ERC20 tokens will launch and pools for them created, so to accept these token pools for fees, you will have to make Governance proposals again and again for each pool separately.

• Let's say token XYZ is a new and powerful token launched
• The token has 5 pools (WETH, USDC, ...)
• If we want to accept fees from all pools for that token, we will have to make a proposal for each pole, separately.

Recommendations

I recommend making the function to set pool fees in batches, where pools are passed to the function as an array, and the function goes to each pool and activates it, this will allow the governance to accept more than one pool in a single proposal.

And instead of modifying the function itself, we can add another function for adding more than one pool for verbosity and flexibility.

---

[NC-02] Values are not checked that they differ from the old values when altering them

In Unistaker::_alterDelegatee and Unistaker::_alterBeneficiary, the new value provided is not checked if it is the same as the old value or not, changing by providing the same value will cause emitting events with no meaning and may cause confusion.

Recommendations

Check that the new address provided either the new delegatee or the new beneficiary equals the old one or not.

---

[NC-03] Unauthorized error has no parameters in V3FactoryOwner

In UniStaker contract, the unauthorized error has two parameters as it is used to authorize both the notifier and the admin.

UniStaker.sol#L73

  error UniStaker__Unauthorized(bytes32 reason, address caller);

But in the case of V3FactoryOwner, the error has no parameters, and it is named unauthorized too.

V3FactoryOwner.sol#L54

  error V3FactoryOwner__Unauthorized();

Recommendations

Either provide a message in the V3FactoryOwner__Unauthorized, or we can change the error name to V3FactoryOwner__NotAdmin, so that it will be easy for frontend devs to handle the error in the UI.

---

[NC-04] Type definition should be at the top

In UniStaker contract, the type definition of DepositIdentifier is presented inside the contract, this is not a good practice, and type definitions are preferred to be at the top of the file.

UniStaker.sol#L31-L32

contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  type DepositIdentifier is uint256;

  ...
}

Recommendations

Put the type declaration in the top of the file

---

[NC-05] Time-weighted contributions staking algorism is activated only after the first reward notified

The staking algorism implemented by synthetix activates only after the first reward notified, what I mean by this is that as long as the first reward has not been notfyied yet, the one who staked on the early will gain the same as the one who staked before norifying the first reward by some minutes.

Since this is how the algorism work, solving this is not ideal, and can cause more issues. But I wanted to point out this here so that devs notice about this thing.

Recommendations

Make the first notify as early as you can.

• Protocol launched
• Stakeholders staked their funds
• Notify rewards as early as you can

---

[NC-06] Fixed payoutAmount may cause some pools unprofitable to claim their fees

In the current implementation of V3FactoryOwner, the payoutAmount is fixed for all pools activated. So the one who is going to claim fees and send rewards will have to pay that amount to collect pool actions fees.

Since pools are not always active, and some pools may have fewer actions (swaps and flash loans), the fees collected by the protocol may not reach this limit forever (fees collected be smaller than the amount needed to be paid).

Recommendations

Since fixing payoutAmount is desired by the design, making a variety of payAmount for pools is not intended by the Dev team. So I recommend not adding pools that have fewer active swaps or interactions

Summary

---

Overview of the UniStaker protocol

UniStaker will be used as an infrastructure for all UniSwap foundation protocols and upcoming protocols. It will be the gate for all Stakeholders to earn yields in WETH token by investing in this protocol and putting UNI token in it.

Rewards will be distributed to the stakes using time-weighted contributions staking algorism.

Fees will be collected from different Uniswap Projects (V3 pools will be activated at this moment) and reward the stakers with that fees.

The protocol fees will be collected by anyone, but the caller should pay an amount of WETH token to fire this function and claim protocol fees earned from Uniswap-V3 pools.

Once claiming the fees, WETH will be sent to The Staker contract, and stakers will earn yields (Amount of WETH).

The admin is the one who can control the amount to be paid to claimFees, the pools that will get activated for protocol fees, and the contracts (notifiers) that can increase the staking amount in Staker contract.

The admin is willing to be the Uniswap Governance contract, so the stakers who hold their staking tokens will not only earn yields by staking, but also will have the power to vote for the changes that can happen that will affect Staking contract (adjusting the payoutAmount, or the pools to colect fees from).

Stakers (people that hold UNI in the staker contract), can claim their rewards, withdraw and take their UNI tokens if they have no plans to stake, and vote on proposals that can be made by the Governance contract to accept or deny a certain change.

---

System Overview

The protocol consists of 2 main Components

1. Staking contract
2. Notifier Contracts

Staking Contract

The staking contract UniStaker.sol has a lot of public functionalities that can be fired by anyone:

• Stake their UNI tokens.
• The staker provides 2 additional addresses (beneficiary and delegatee)
  • beneficiary is the address that will receive rewards, the user can make this his address or another address.
  • delegatee is that address that will have the power of votes for UNI token.
• Stakes happen using IDs, so the same user can stake more than one time, and he will be the owner of two different deposit IDs. this will be helpful if he wants to separate his tokens for different beneficiaries, and delegatees.
• The user who staked for a given deposit ID, can increase the value of his deposit ID by staking more UNI tokens to the same deposit ID.
• The user can withdraw his staking tokens from a given deposit ID anytime.
  • If the user staked more than one time and has more than one deposit ID, he can not unstake all his tokens at once. He needs to call withdraw two times to withdraw, providing deposit ID for each, as the withdrawal happens by providing single deposit ID.
• The beneficiary address can claim his rewards.
  • beneficiary can either claim all his rewards at the time of calling or leave them, he can not withdraw part from his earnings.
  • The depositor who staked the token can not claim beneficiary rewards, he can just unstake tokens, preventing the beneficiary from earning.
• The deposited (The owner of a given deposit ID), can change his deposit ID configurations, this includes:
  • Changing the beneficiary address, that will receive rewards.
  • Changing delegatee address that has the power of voting.
  • Increasing the amount of deposit ID balance (staked token), by staking more tokens to the same deposit ID.
• All the functions called by the users including staking, withdrawing, claiming, or configuration of a certain deposit ID, can be called without a direct call from the owner. The owner of the deposit ID can sign the message, and allow 3rd party to fire his transaction. Staking a new deposit ID allows this functionality too.

Other functions are only callable by the admin of the contract these include:

• Adding a new reward notifier, which is the address that can increase the staking reward amount.
• Changing the admin address.

Notifier Contract (V3FactoryContract)

The system is designed to be able to have more than one notifier contract. In the context of this codebase, there is only one notifier contract (V3FactoryOwner), which is responsible for claiming fees from Uniswap v3 DEX pools.

V3FactoryOwner.sol characteristics:

• It will be the owner of the uniswapV3 factory contract, which is used to deploy new pools.
• Have an admin role, which is planned to be set to the Uniswap Governance, and it can do the following:
  • Adding new fee/tick support for the future pools that will be deployed (used to manage concentrated liquidity).
  • Enabling protocol fees to a given uni-v3 pool.
  • Changing the admin of the contract.
• Have a function that allows anyone to claim the fees from a given pool, but the caller should pay an amount of WETH first, to claim the pool fees.

UNI tokens holder (DelegateSurrogate)

Once stakers stake their tokens, they provide the delegatee address which will have the power of the tokens held by the stakers.

The tokens themselves will not be with the delegatee address, just the voting power will be set to delegatee (this is achievable as UNI is a vote token). The tokens will be sent to a surrogate contract address, which will be like a safe place for Staked tokens. And these tokens can be withdrawn from this contract anytime from the UniStaker contract When withdrawing,

System Architecture

The system works 100% on-chain, with no oracles, and no Web2 development integrations.

• The Staking Contract (UniStaker) uses time-weighted contributions staking algorism, which distributes rewards according to the amount staked and the duration these staked tokens were in the contract.
• Distributing Staking rewards are restricted to the notifier contracts.
• The notifier contract can only distribute reward after transferring the amount of the reward to the UniStaker.
• Users are incentivized to transfer rewards, as they will claim the protocol fees collected from the univ3-pool passed.
• Stakers will have the power to vote for Uni Governance contract using their UNI voting power which is set to delegatee

---

Documentation

The documentation was impressive. UniSwap Devs provided detailed information that helps us in our auditing process, this includes:

• Detailed explanation of the system.
• Providing diagrams that facilitate understanding of the protocol.
• Pointing out the known issues, and the points that we should focus on.
• Detailed comments for every function in the .sol files, which explain what is the purpose of the function.

---

‎Centralization risks‎

The system will face centralization risks at the beginning of its deployment. But since it is planned to make the admin of the Staker contract and the notifier contracts the Governance contract of the Uniswap, The centralization risk will not stay.

---

My view on the project

The project is impressive. I liked it. The project is insane in my opinion, because of the following features:

• The system will be controlled by the Uni Governance contract, which uses UNI token for proposing and voting. and this token which will control the governance contract. So the stakers themselves will be able to control how the system they are investing in should work, and this is a good point.
• Users' funds Staked tokens are safe, and they can receive them back anytime, even the admin can not control their tokens.
• The system is designed with great flexibility, where users can sign messages, without direct contact with the protocol.
• The reward token is WETH which is a well-known token, packed by native ETH.

---

Time spent on analysis

I spent 6 days auditing, reporting, and analyzing the codebase, with a total hours ~30 Hours.

Time spent:

30 hours