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: 43/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
Brahma Console v2 is a platform that helps people use decentralized finance (DeFi) more easily and safely. It does this by providing a layer of automation and risk management on top of smart contract wallets. It provides sub-accounts, which isolate user interactions to reduce risk. The Console is built on the principle of non-custodiality, which means that users shouldn't have to give up control of their funds to execute transactions.
It consists of four major account types:
safeModeratorOverridable
if the owners want to;SafeModerator
as a safeguard;safeModerator
and controlled by the console account;AddressProvider
contract. It does this by providing a host of helper functions to help in these interactions;TransactionValidator
contract;safeModerator
contract. On top of this, it also does a check to confirm if an owner decides override the it as a safeguard and/or the ConsoleFallbackHandler
as the fallback handler.executorPlugin
for sub acoounts module executions.ModuleManager
and GuardManager
to set up modules and safeguard states. This helps the accounts bypass the selfAuthorized checks that exist on the actual Safe.CompatibilityFallbackHandler
, but it also ensures that policy validation is performed for ConsoleAccounts and sub-accounts that have policy validation enabled;execTransaction
is called by a console account without policies and safeguard in the GnosisSafe.sol contract. The desired call datas execute without any safety checks;
execTransaction
is called by a console account with policies and safeguard. Here, the execTransaction
function performs the needed safety checks before and after executing the transactions;
execTransaction
calls checkTransaction
function in the SafeModeratorOverridable.sol;checkTransaction
function calls the validatePreTransactionOverridable
function in the TransactionValidator.sol;validatePreTransactionOverridable
checks if the guard isn't being removed with the help of the _isConsoleBeingOverriden
. This checks to see if the required guard removal conditions are met. As, the guard isn't being removed, the _validatePolicySignature
is called._validatePolicySignature
calls the isPolicySignatureValid
in the PolicyValidator.sol contract. This validates the policyHash and the Trusted Validator Signature.execTransaction
function calls the checkAfterExecution
function in SafeModeratorOverridable.sol contract. This calls the validatePostTransactionOverridable
in the TransactionValidator.sol contract.Transaction function flow
execTransaction → checkTransaction → validatePreTransactionOverridable ==> _isConsoleBeingOverriden (No) ==> _validatePolicySignature → isPolicySignatureValid → execTransaction (desired calldatas are executed) → checkAfterExecution → validatePostTransactionOverridable
Transaction contract flow
GnosisSafe → SafeModeratorOverridable → TransactionValidator <==> TransactionValidator <==> TransactionValidator → PolicyValidator → GnosisSafe → SafeModeratorOverridable → TransactionValidator
execTransaction
is called by a console account with policies and safeguard, while also disabling the safeguard - This occurs almost as above with the added caveat that the safeguard is overriden. The action of overriding the safeguard skips the policy validation, and disables the guard by setting guard address to 0. However unlike stated in the readme, after guard is disabled. It doesn't seem to perform the checkAfterExecution. This is because before performing checkAfterExecution, there's a check to see if guard address isn't 0. If 0, the checkAfterExecution is skipped.
Transaction function flow
execTransaction → checkTransaction → validatePreTransactionOverridable ==> _isConsoleBeingOverriden (Yes) == _validatePolicySignature =→ isPolicySignatureValid → execTransaction (desired calldatas are executed, setGuard(address(0)))
Transaction contract flow
GnosisSafe → SafeModeratorOverridable → TransactionValidator <==> TransactionValidator → TransactionValidator =→ PolicyValidator → GnosisSafe
execTransactionFromModuleReturnData
is called by the console account via the sub accouunt using the ModuleManager.sol. The function executes the desired calldata and returns returnData to ConsoleAccount.
execTransaction
is called by operator through the sub-account - This tranaction occurs just like console account exectransaction (see â„–2) above. The only difference is that instead of the checks being conducted by the SafeModeratorOverridable.sol contract, it is conducted by the SafeModerator.sol instead.
execTransaction
calls checkTransaction
function in the SafeModerator.sol;checkTransaction
function calls the validatePreTransaction
function in the TransactionValidator.sol;_validatePolicySignature
. which calls the isPolicySignatureValid
in the PolicyValidator.sol contract. This validates the policyHash and the Trusted Validator Signature.execTransaction
function calls the checkAfterExecution
function inSafeModerator.sol contract. This calls the validatePostTransaction
in the TransactionValidator.sol contract. This checks the security config by calling the _checkSubAccountSecurityConfig
function. It makes sure guard, fallbackhandlers and the console has not be disabled as module.Transaction function flow
execTransaction → checkTransaction → validatePreTransaction ==> _validatePolicySignature → isPolicySignatureValid → execTransaction (desired calldatas are executed) → checkAfterExecution → validatePostTransaction ==> _checkSubAccountSecurityConfig
Transaction contract flow
GnosisSafe → SafeModerator → TransactionValidator <==> TransactionValidator → PolicyValidator → GnosisSafe → SafeModerator → TransactionValidator <==> TransactionValidator
executeTransaction
is called the executor on the ExecutorPlugin.sol enabled as module on the sub-account.
executeTransaction
, this validates the signer executor by calling the _validateExecutionRequest
, where it's determined if the executor is valid for the sub-account by checking with the ExecutorRegistry.sol, validates the executor's signature and validates the sub-account's policy by calling the validatePreExecutorTransaction
function in the TransactionValidator.sol function.validatePreExecutorTransaction
function, the policy signature is validated by the _validatePolicySignature
. which calls the isPolicySignatureValid
in the PolicyValidator.sol contract._executeTxnAsModule
function.validatePostExecutorTransaction
function on TransactionValidator. to check that SafeModerator hasn't been removed as guard on SubAccount and ConsoleAccount hasn't been removed as module on SubAccount.Transaction function flow
executeTransaction → _validateExecutionRequest → validatePreExecutorTransaction ==> _validatePolicySignature → isPolicySignatureValid → executeTransaction (desired calldatas are executed)(_executeTxnAsModule) → validatePostExecutorTransaction → _checkSubAccountSecurityConfig
Transaction contract flow
ExecutorPlugin <==> ExecutorPlugin (checks with ExecutorRegistry) → TransactionValidator <==> TransactionValidator → PolicyValidator → ExecutorPlugin → TransactionValidator <==> TransactionValidator
The codebase is of high quality, well-structured, and would-be large functions are carefully divided into separate contracts. Required functions are either directly called or delegate called when needed. The codebase is well-commented, and its documentation is easy to follow. There are also unit tests for the contracts, which is very commendable. Error handling is fine. The codebase uses a mixture of custom, require, and revert errors. It's better to stick to one format, preferable custom errors as they're gas effiecient. In a host of cases, the errors weren't very descriptive, some without messages, some with cryptic messages like "GS102". An error also appears to be unused. Events were also well-handled, some events didn't fully emit needed parameters, some didn't have a lot of declarations or were well-handled. In general, NatSpec and the Style guide were followed, not 100%, but just enough to make the contract look well-organized. We recommend using linters and static analysis tools that can help note and fix these issues before deployment.
The risk of centralization seems to be limited to the Brahma-owned governance and guardian. Any malicious actions (intended or not) can pose a risk to the protocol and its users. Some contracts inherit third-party contracts like OpenZeppelin, Solidity, GnosisSafe contracts. Any issue found in these inheritances might pose a threat to the protocol. One thing to also note is that although wallets and sub-accounts can be added, they cannot be removed. Not sure if that is intended by the sponsor, but it might cause a potential issue in case of some problem to keep the console secure.
We started the audit process by thoroughly reviewing the provided documentation, tests, noting various important parts, and asking the sponsors questions. We used static analysis tools and linters to audit the codebase, noted the bot reports, and went about with the manual code inspection. We carefully inspected each section of the codebase, tested possible attack vectors, focusing both on the sponsor's interested areas of attack (through imported wallets) and areas we deemed important. We took notes of our findings, tested them out, and prepared our analysis. Overall, the console contract looks to be well-written and doesn't have a lot of serious issues. We recommend the team goes through these, make appropriate modifications where needed before deployment.
36 hours
#0 - c4-pre-sort
2023-10-22T21:21:11Z
raymondfam marked the issue as sufficient quality report
#1 - alex-ppg
2023-10-27T13:10:59Z
This report feels more like a rudimentary analysis of individual contracts rather than a solid overview of the Brahma system from a security perspective.
#2 - c4-judge
2023-10-27T13:11:12Z
alex-ppg marked the issue as grade-b