Skip to content

rocket-pool | Ethereum | 12.12.2024#45

Open
flbouchut wants to merge 8 commits intodeficollective:mainfrom
flbouchut:main
Open

rocket-pool | Ethereum | 12.12.2024#45
flbouchut wants to merge 8 commits intodeficollective:mainfrom
flbouchut:main

Conversation

@flbouchut
Copy link

No description provided.

@vercel
Copy link

vercel bot commented Nov 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
defiscan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 19, 2025 8:02pm

@yvesbou
Copy link
Collaborator

yvesbou commented Nov 28, 2024

awesome @flbouchut can you please in a first step include the scope (ie. all addresses you're going to scan and audit)

here:

Technical Analysis

Contracts

Contract Name Address
contract 1 0x123
contract 2 0x456

feel free to start 🙏

@flbouchut
Copy link
Author

Thanks for the feedback @yvesbou, there are more than 60 contracts deployed on mainnet for Rocket Pool: contracts-addresses.
We'll precise the scope of the review once we have a clearer understanding of which ones are relevant to include.

@yvesbou
Copy link
Collaborator

yvesbou commented Dec 10, 2024

here is something cooking, already look forward to review 🙏 @flbouchut

Copy link
Collaborator

@yvesbou yvesbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @flbouchut

Thanks the submission already looks really good. For a defi user that does not know the technical details we lack verifiability of the claims in the upgradability section. We thus require more details in the technical section.

upgradability

This section at this point looks good. Potentially after improving the technical section we can pinpoint required changes for capturing upgradability impacts.

According to our framework upgradability risk is expressed by the following vectors

  • theft of user funds
  • loss of unclaimed yield
  • material changes to the protocol (kyc, blacklist, fees, etc.)

The goal of the overview upgradability section is to give a short summary why one of the above risks exist.

technical section

diagram

Can you please introduce an architectural diagram of the current system or point to an existing one that shows the following. (We require this because of the large off-chain (beacon chain) mechanics.)

Show how funds flow, where are funds exactly located, who controls the funds, how parameter and contract upgrades change the potential flow of actions.

impact column in permission table

Can you please improve the cells of the impact column such that the respective functions responsible for upgradability risk explain the vector in more detail. A simple framework that helps is as follows

  • First sentence: what it does technically, e.g "It assigns a new address to the owner variable"
  • Second: what is the impact within the system, e.g "The owner is permissioned to raise fees"
  • Third: Imagine faulty or malicious action, e.g "The malicious owner could raise fees to 100%, redirecting all future yield.

Additionally we found the following contracts not to be scanned

  • rocketAuctionManager
  • addressQueueStorage
  • addressSetStorage
  • rocketMerkleDistributorMainnet
  • rocketMinipoolBase
  • rocketMinipoolBondReducer
  • rocketMinipoolDelegate
  • rocketMinipoolFactory
  • rocketMinipoolManager
  • rocketMinipoolPenalty
  • rocketMinipoolQueue

Please include them in the report (ie. scan them)

@flbouchut
Copy link
Author

Hi @yvesbou

It's been a while! (winter holidays and other things...)

First, I've added the requested contracts and included them in the analysis. At the same time, I reworked the Permission functions table following your guidance.

As for the technical analysis and diagram, there isn't a complete technical architecture diagram online, and creating one with the 60 contracts and their relationships would result in an overly complex and impossible to understand diagram. Instead, I've drawn a simplified one that focuses on the governance processes. Additionally, I've expanded the upgradeability section to clarify that while protocol funds are not at risk, rewards and protocol settings could be affected by malicious actions.

Let me know what do you think of the current state of the review and I would be happy to make further updates or adjustments wherever needed.

@yvesbou
Copy link
Collaborator

yvesbou commented Jan 20, 2025

Hi @flbouchut
Happy new year! We are going to have a closer look and come back with feedback as soon as possible 🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants