Platform: Code4rena
Start Date: 13/10/2023
Pot Size: $31,250 USDC
Total HM: 4
Participants: 51
Period: 7 days
Judge: 0xsomeone
Id: 295
League: ETH
Rank: 51/51
Findings: 1
Award: $14.47
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: niroh
Also found by: 0xDetermination, 0xSmartContract, 0xbrett8571, 0xdice91, 0xweb3boy, Bauchibred, Bube, DadeKuma, JCK, K42, LinKenji, Myd, SAAJ, ZanyBonzy, albahaca, castle_chain, catellatech, digitizeworx, emerald7017, fouzantanveer, hunter_w3b, invitedtea, m4ttm, rahul, xiao
14.466 USDC - $14.47
The audit initiated with a high-level overview of the Brahma ecosystem. Steps included referencing the [previous audit by Ackee](# Analysis
The audit initiated with a high-level overview of the Brahma ecosystem. Steps included referencing the previous audit by Ackee, leveraging static analysis tools, and manually reviewing the 800+ sLOC. The final phase consisted of interactive developer sessions on Discord, deepening comprehension of unique implementations and promoting discussions around potential vulnerabilities and significant observations.
AddressProvider: Manages and updates addresses used by other system contracts. Specifically, it handles registries and authorized contracts. Managed by an address representing governance. Interaction with this contract is facilitated through the AddressProviderService contract.
Registries: The protocol employs three registries:
SafeDeployer: Oversees Safe account deployment and configuration.
SafeModerator & SafeModeratorOverridable: Validates transactions intended for subaccounts. The latter is designed for console accounts.
TransactionValidator & PolicyValidator: Offer validation hooks, examining policies, and signatures.
ExecutorPlugin: Represents a Safe module and additional execution capabilities.
Actors: Outlines the system's roles and permissions.
Safe, Console & Subaccount: Detail the architecture and functionalities of each.
Executor: Defines accounts permitted to use the ExecutorPlugin.
Brahma Governance: Describes the account that can add, but not update, authorized addresses or registries.
Upholding trust assumptions is pivotal for the project's longevity just as any other web3 protocol, NB: the protocol is well-designed and leverages best practices of trustless protocols. However, the governance is in charge of the authorized addresses registry which can affect users positively & also negatively.
Another tricky one is the fact that a trust notion is placed on validators and assumption is made that they always will validate the transactions correctly with the policy, and the policy will always prevent the sub account from doing unintended transactions, seems like a far stretch but it still entails a bit of centralization risk in it's ranks.
The level of testability exhibited by the system is both extensive and praiseworthy. Achieving 100% test coverage is an admirable accomplishment. However, it's recommended to diversify test scenarios, particularly from the user's perspective. For instance, the team should explore potential outcomes when a user attempts to integrate with the system using a counterfactual account. It would be beneficial to understand how the current isValidSignature implementation interacts in such contexts among other user-centric scenarios.
Many security experts prefer using Visual Studio Code augmented with specific plugins. While the popular Solidity Visual Developer has already been integrated with the protocol, there's room for incorporating other beneficial tools.
Although the report commends the 100% test coverage, it's imperative to understand that it doesn't guarantee a bug-free system. The team should consider adding more tests, especially those reflecting real-world user interactions with the protocol.
The current approach to event tracking could be optimized. Notable gaps include missing events or those that appear inconsistently placed. It's advised to employ a more deliberate and structured approach to event utilization.
Having multiple eyes scrutinizing a protocol can be invaluable. More contributors can significantly reduce potential risks and oversights.
Though the current code documentation meets an acceptable standard, there's room for enhancement. Comprehensive documentation can aid in clarity and ease of onboarding for new developers.
While the extensive use of factories and libraries introduces additional complexity, it's recognized that such intricacies might be unavoidable, especially given the integration with platforms like Gnosis's Safe wallets.
There's a need to improve the naming conventions for contracts, functions, and variables. In several instances, the names don't resonate with their respective functionalities, leading to potential confusion.
My attempt on reviewing the Brahma spanned 15 hours distributed over 3 days:
The codebase was a very great learning experience, though it was a pretty hard nut to crack, being that it's like an update contest since bugs have already been spotted and mitigated from the previous contest.
During the course of my security review, I encountered a few interesting reports regarding how things would go wrong for users who sign a transaction from their accounts while these accounts exist counterfactually and how this wouldn't work. Additionally another interesting one is centred around how the magic value being used for the EIP 127! return value is different from what's been specified in the EIP and would lead to compatibility issues, since even correct signatures based on the EIP would be deemed invalid.
), leveraging static analysis tools, and manually reviewing the 800+ sLOC. The final phase consisted of interactive developer sessions on Discord, deepening comprehension of unique implementations and promoting discussions around potential vulnerabilities and significant observations.
AddressProvider: Manages and updates addresses used by other system contracts. Specifically, it handles registries and authorized contracts. Managed by an address representing governance. Interaction with this contract is facilitated through the AddressProviderService contract.
Registries: The protocol employs three registries:
SafeDeployer: Oversees Safe account deployment and configuration.
SafeModerator & SafeModeratorOverridable: Validates transactions intended for subaccounts. The latter is designed for console accounts.
TransactionValidator & PolicyValidator: Offer validation hooks, examining policies, and signatures.
ExecutorPlugin: Represents a Safe module and additional execution capabilities.
Actors: Outlines the system's roles and permissions.
Safe, Console & Subaccount: Detail the architecture and functionalities of each.
Executor: Defines accounts permitted to use the ExecutorPlugin.
Brahma Governance: Describes the account that can add, but not update, authorized addresses or registries.
Upholding trust assumptions is pivotal for the project's longevity just as any other web3 protocol, NB: the protocol is well-designed and leverages best practices of trustless protocols. However, the governance is in charge of the authorized addresses registry which can affect users positively & also negatively.
Another tricky one is the fact that a trust notion is placed on validators and assumption is made that they always will validate the transactions correctly with the policy, and the policy will always prevent the sub account from doing unintended transactions, seems like a far stretch but it still entails a bit of centralization risk in it's ranks.
The level of testability exhibited by the system is both extensive and praiseworthy. Achieving 100% test coverage is an admirable accomplishment. However, it's recommended to diversify test scenarios, particularly from the user's perspective. For instance, the team should explore potential outcomes when a user attempts to integrate with the system using a counterfactual account. It would be beneficial to understand how the current isValidSignature implementation interacts in such contexts among other user-centric scenarios.
Many security experts prefer using Visual Studio Code augmented with specific plugins. While the popular Solidity Visual Developer has already been integrated with the protocol, there's room for incorporating other beneficial tools.
Although the report commends the 100% test coverage, it's imperative to understand that it doesn't guarantee a bug-free system. The team should consider adding more tests, especially those reflecting real-world user interactions with the protocol.
The current approach to event tracking could be optimized. Notable gaps include missing events or those that appear inconsistently placed. It's advised to employ a more deliberate and structured approach to event utilization.
Having multiple eyes scrutinizing a protocol can be invaluable. More contributors can significantly reduce potential risks and oversights.
Though the current code documentation meets an acceptable standard, there's room for enhancement. Comprehensive documentation can aid in clarity and ease of onboarding for new developers.
While the extensive use of factories and libraries introduces additional complexity, it's recognized that such intricacies might be unavoidable, especially given the integration with platforms like Gnosis's Safe wallets.
There's a need to improve the naming conventions for contracts, functions, and variables. In several instances, the names don't resonate with their respective functionalities, leading to potential confusion.
My attempt on reviewing the Brahma spanned 15 hours distributed over 3 days:
The codebase was a very great learning experience, though it was a pretty hard nut to crack, being that it's like an update contest since bugs have already been spotted and mitigated from the previous contest.
During the course of my security review, I encountered a few interesting reports regarding how things would go wrong for users who sign a transaction from their accounts while these accounts exist counterfactually and how this wouldn't work. Additionally another interesting one is centred around how the magic value being used for the EIP 127! return value is different from what's been specified in the EIP and would lead to compatibility issues, since even correct signatures based on the EIP would be deemed invalid.
15 hours
#0 - c4-pre-sort
2023-10-22T21:19:11Z
raymondfam marked the issue as sufficient quality report
#1 - alex-ppg
2023-10-27T13:17:08Z
The report contains its body duplicated twice. Its body is mainly composed of generic text that does not really provide an analysis of the Brahma system except for the Contract Architecture Overview.
#2 - c4-judge
2023-10-27T13:17:13Z
alex-ppg marked the issue as grade-b