UniStaker Infrastructure - ihtishamsudo's results

Unistaker Infrastructure Analysis Report

Preface

This report is a summary of the analysis of Unistaker Infrastructure Protocol's Codebase.

• This report is not an extension of the documents/Info provided by Unistake Infrastructure Protocol
• This report provides a high-level overview of the codebase and its security implications and some edge cases missed by developers and some suggestions to improve the codebase.
• This report aims to provide value to the sponsors and the developers of Unistake Infrastructure Protocol.

General Overview About Protocol

Uniswap Infrastructure proposed enable Uniswap Governance to regulate protocol fees for Uniswap V3, granting them the power to adjust fees while relinquishing control over fee assets. Instead, fees collected are automatically distributed to UNI holders who stake their tokens. Stakers receive rewards proportional to their stake in the total UNI pool, which are not directly in the form of fee tokens. Stakers can delegate their voting rights and designate a beneficiary address to receive rewards. Additionally, the system has the potential for future expansion to include additional revenue sources for staking rewards.

Important Disclosure For Analysis ⚠️

As the uniswap developer developed this protocol, so, certainly they must have knowledge about their protocol how they developed and what's in their protocol. So, I would most probably avoid all the overview summary that protocol developer already know and respectfully focus only on the things that might be helpful for developers and weak spots that might be overlooked by developers and potentially help in fixing those odd things.

Architectural Improvement

First I would like to detail below all the architectural imporvements if protocol have done that way it would have been good.

1. Overall Structure Of Protocol

Protocol is structured in simplistic way. As there are three main contracts in the protocol: DelegationSurrogate, Unistaker, V3FactoryOwner. DelegationSurrogate Hold Governance Tokens and delegate power to a delgatee. Unistaker is main contract that handle the distribution of rewards to staker. V3FactoryOwner is the owner of the Uniswap v3 Factory.

• Improvements that should be made

The main thing i find the most lacking was the contracts are not upgradable means there is no possibility for enhancing or adding new features or fixing bugs. By Introducing upgradeability patterns such as proxy contracts or upgradeable smart contracts one can enable future enhancements and bug fixes without disrupting the existing system. So, I would suggest to make the contracts upgradable so that in future if there is any bug or any new feature to be added then it can be done without any hassle.

In V3FactoryOwner Contract, Protocol should have used Role-Based Access Control (RBAC) : Instead of having a single admin, protocol could use a role-based access control system. This would allow protocol to assign different roles with different permissions. For example, protocol could have a role for setting the admin, another for setting the payout amount, etc.

What protocol Did Unique?

1. Trustless Fee Distribution - The revenue generated by protocol fees is trustlessly distributed to UNI token holders who stake and delegate their tokens.
2. Continuous Fee Auctioning - Fees accrued by each pool are continually auctioned to any entity willing to pay a fixed amount of a designated token to the staking contract in exchange for the fee tokens accrued by a given pool.
3. Reward Benificiery Designation - Stakers can designate the beneficiary of their staking rewards, allowing any given deposit to earn rewards on behalf of any designated address.
4. Per-Deposit Tracking - Stake is tracked on a per-deposit basis, allowing stakers to add to or withdraw UNI from a deposit balance and alter governance delegation and reward beneficiary associated with that deposit.
5. Flexibility For Future Rewards - While Uniswap V3 protocol fees are the initial source of rewards, the system is designed to accept rewards from any number of addresses designated by the admin, with the possibility of adding more reward sources in the future.

New Features That Can Be Added

1. Multi-Asset Awards - Expand the reward system to support multiple assets beyond UNI tokens, providing stakers with additional incentives and diversification opportunities.
2. Dynamic Fee Adjustment - Implement a mechanism for dynamic adjustment of protocol fees based on factors such as liquidity depth, trading volume, or market conditions to optimize fee revenue.
3. Community-Driven Rewards Sources - As we love and believe in decentralization so Protocol should allow community members to propose and vote on additional reward sources beyond protocol fees, fostering community involvement and decentralization.

Serious Flaws That Developers Overlooked

It was amazing to see more than 5k+ lines of code of testing for just 1k+ lines of sLOC. Protocol developer tried their best to cover all the possible scenarios edge cases and attack vectors but there are still some edge cases that were'nt even in the mind of developers/testers that's why they didn't even write test cases for those particular cases.

1. Griefing On Staking Via Signature - As protocol is using permit functionality that is similar to OpenZepplin ERC20 Permit functionality, firstly the protocol didn't used OZ library and build similar but their own permit functionality and secondly they implemented a similar mechanism but failed to implement the security consideration suggested by OpenZepplin regarding permit() functionality. I've submit Med demonstrating this attack Wherease an attacker can monitor transaction in memepool extract the signature and call with the direct permit function so the call permitWithStake & permitWithStakeMore would revert. So, I would suggest to implement the security consideration suggested by OpenZepplin regarding permit() functionality.

2. No Pausable Functionality - As protocol is all dealing with the rewards and dealing of those rewards to stakers and delagators, it means there are lot of transactions happening only regarding funds but there is no pausable functionality incase of attack on the system on mainnet. If blackhat found any vulnerability on Mainnet then there will be there will be no way to stop the attack. So, pausable library by OZ must be implemented in the protocol and applied on critical functions such as staking or withdrawal ones as pufferFinance has protected themselves from the attack by pausing the vulnerable swap functionality on mainnet, same machanism is needed to be implemented here.

3. No Upgradable Contracts - As I've mentioned above before, upgradeable contracts is must to have for crucial protocols that handles funds and rewards. If there is any bug disclosed on Mainnet, there will be no way to implement a bug fix because contracts are no upgradable. So, I would suggest to make the contracts upgradable so that in future if there is any bug it could be fixed.

4. No Reentrancy Protection - withdraw and withdrawOnBehalf can be exposed to reentrancy attack if there is any edgecase exposed to reentrancy attack. Where any caller on behalf of someone can withdraw rewards again and again. So, protocol should use Reentrancy Guard to protect the functions from reentrancy attack.

Centralisation Risks

One of the biggest centralisation risk is the admin, because an admin is playing crucial role in setting important parameters of functionalities in protocol such as

• setting new admin
• setting payout amounts
• setting protocol fee
• setting wether prtocol fee is enable or not etc.

If the admin got compromised this will lead to the failure or bricking of the entire protocol. I would recommed using a multisig for all the admin tasks.

Some Weak Spots In Codebase

In this section i will try to cover all the @audit tags that i put in the codebase weak spots and don't find it to submit as Low in the report, so mentioning all here so developers can improve those.

As protocol didn't imported much helping libraries from Openzeppllin, and everything the built with their own mechanism.

1. Protocol should use 2StepOwnership transfer library from openzepplin while setting critical new protocol addresses such as new admin, new payout address etc.

2. Complex maths has been done in the protocol which could lead to precision loss and overflow/underflow. So, I would suggest to use SafeMath library from OpenZepplin.

3. Rounding errors testing should be properly done. As most of the hacks are happening because of rounding errors so a propor testing particularly for rounding errors should be done. A free Open-source tool Roundme by TrailofBits must be use for testing rounding errors in cruicial functions such as rewardPerTokenAccumulated, notifyRewardamount & UnclaimedRewards etc for finding potentional rounding errors.

4. Even though protocol is using only UNI token but protocol might want to add more tokens in future so using maximum supply while approve is not a good idea. So, I would suggest to use custom allowance instead of type(uint256).max in approve function so delegatee don't abuse this functionality

Systematic Risk

1. Main Unistaker contract would not work when the protocol wished to exapand the protocol to other chains as well just because the protocol using Immutable WETH as payout token and WETH has different contract addresses on different chains so if protocol want to expand on other chains as well make sure to look out the possible scanarios and how things can be mitigated before deploying across other chains.

2. Contract has several immuntable variables such as FACTORY and PAYOUT_TOKEN. If these need to be changed after deployment for any reason, it would require deploying a new contract.

Attack Vectors and Ideas

1. Signature Forgery

The function stakeOnBehalf relies on the _revertIfSignatureIsNotValidNow function to verify the signature of the depositor. If there's a flaw in the signature verification process, an attacker could forge a signature and stake tokens on behalf of another user without their consent.

2. FrontRunning

As Explained Deeply above that functions depending on signatures can be frontrunned badly

3. Invalid Validation by Mistake

Most of The functions do not perform any checks on the _amount parameter. If a user accidentally enters a very large number, it could result in unexpected behavior or loss of funds.

4. Time Depndency Manipulation

Protocol function lastTimeRewardDistributed() depending on block.timestamp, as we know block.timestamp can be manipulated by miners so an attacker to some extent so manipulation in this specific function can result into incorrectness in my other conponents of the protocol that are validating by depending on this function.

High Level System Overview

Providing High-Level system overview just for the sake of understanding for wardens reading this report as well as protocol developer to understand the system in a better way.

Protocol Contracts Calls & Interaction Flow

1. Unistaker Contract

This contract manages rewards distribution in ERC20 tokens deposited by authorized notifiers. Staking involves locking ERC20 governance tokens, which can be delegated for voting power, for a set period. Stakers can delegate voting power and designate a beneficiary address. Inspired by Synthetix StakingRewards.sol, rewards are streamed over time based on stake share. Stakers earn rewards only while tokens are staked and can add or withdraw stake anytime. Reward duration restarts with each new reward, adjusting the streaming rate accordingly.

<a href="https://imgur.com/ROjYLxz"><img src="https://i.imgur.com/ROjYLxz.png" title="source: imgur.com" /></a>

2. V3FactoryOwner Contract

This contract functions as the owner of the Uniswap V3 factory and is governed by an admin. The admin possesses exclusive authority to invoke privileged methods on the V3 factory and its deployed pools through passthrough methods. This authority extends to enabling fee amounts on the factory, setting protocol fees on specific pools, and appointing a new admin.

<a href="https://imgur.com/7Yl8Gf9"><img src="https://i.imgur.com/7Yl8Gf9.png" title="source: imgur.com" /></a>

3. DelegationSurrogate Contract

This straightforward contract serves the sole function of holding governance tokens for users and delegating their voting power to a specific delegatee. This is necessary because each address can delegate its entire token weight to only one delegatee at a time. Consequently, when a contract holds governance tokens for various token holders in a pool, these holders are usually deprived of their governance rights.

<a href="https://imgur.com/wjKV8Z1"><img src="https://i.imgur.com/wjKV8Z1.png" title="source: imgur.com" /></a>

Appraoch Taken in evaluating Codebase

Joined contest after 5 days

First Day

• Head over to C4 Audit page, to get the overview and how this protocol works
• Cloned the Repo and Build Contracts

Second Day

• Skimmed The Codebase to get an overview
• Found a Potential HM while skimming
• Added Inline Bookmark to report that vulnerability

Third Day

• Deep Depth Review Of Codebase
• Compare Codebase with docs
• Added audit tags on odd things
• Connected Weak Dots

Fourth Day

• Draw Mental Models of How Protocol Interaction Flows
• Ran Static Analysis Both Slither and Aderyn to find vulnerable pattern
• Filtered Out The Inline BookMarks

Last Day

• Submit potential Medium Finding
• Cleared out Audit Tags
• Write Analysis Report
• Finding and Analysis Submitted

Total Time Spent

25 Hours

Time spent:

25 hours