We audited the forta-network/forta-firewall-contracts repository at commit edf03cd and the forta-network/forta-attester-controller-contracts repository at commit 8685148.
In scope were the following files:
forta-firewall-contracts
└── src
├── AttestationForwarder.sol
├── CheckpointExecutor.sol
├── ExternalFirewall.sol
├── FirewallAccess.sol
├── FirewallPermissions.sol
├── Firewall.sol
├── InternalFirewall.sol
├── ProxyFirewall.sol
├── Quantization.sol
└── SecurityValidator.sol
forta-attester-controller-contracts
└── src
├── AttesterControllers.sol
├── Errors.sol
├── Inbox.sol
├── Registry.sol
├── RoleManager.sol
└── Whitelist.sol
The Forta Firewall is an innovative security solution designed to enhance the protection of DeFi protocols operating on EVM networks. It integrates off-chain exploit detection capabilities with on-chain smart contract security measures. The system works by intercepting and screening transactions before they are executed on the blockchain, allowing for the detection and prevention of potential exploits or malicious activities.
At its core, the Forta Firewall utilizes a system of checkpoints within smart contracts. These checkpoints act as configurable security gates that can activate under specific conditions, such as when a certain transaction parameter exceeds a predefined threshold. When a checkpoint is triggered, it communicates with a validator contract to verify if the transaction has been attested as safe by off-chain security services. The transactions which do not receive attestations revert by default.
Attestations can be stored in two ways:
storeAttestation() function, which utilizes SSTORE to persist the attestation in contract storage.saveAttestation() function, which leverages the more gas-efficient TSTORE/TLOAD opcodes. This method stores the attestation in transient storage, making it available throughout the transaction's execution without incurring the higher gas costs associated with permanent storage writes.This architecture allows for real-time security checks without significantly impacting the performance or gas costs of normal operations. By combining off-chain analysis with on-chain enforcement, the Forta Firewall provides a robust and flexible security layer for DeFi protocols.
The Firewall smart contract is the core component of the Forta Firewall system. It implements the security checkpoint logic, receives attestations, and controls the execution flow based on security validations. The contract includes functions for setting and managing checkpoints, saving attestations, and executing security checks.
Key features of the Firewall contract include:
Quantization is a crucial feature implemented in the Firewall to address value fluctuations between attestation creation and on-chain execution. The Quantization library provides the quantize() function which rounds input values to predetermined scales before use in checkpoint calculations, ensuring consistency between off-chain attestations and on-chain execution, even with slight value changes. This is particularly important in DeFi contexts where asset values can change rapidly.
The Forta Firewall system offers three main implementation variations to accommodate different integration needs and constraints of DeFi protocols. Each variation provides the core firewall functionality while offering unique characteristics for deployment and interaction. The choice between these variations depends on factors such as the existing contract structure, upgrade mechanisms, and the desired level of integration.
The Proxy Firewall is an approach that leverages the proxy pattern common in upgradeable smart contracts. Its characteristics include:
The Internal Firewall is suitable for protocols that can directly incorporate firewall logic into their contracts. Key features include:
The External Firewall is designed for scenarios where direct modification of the target contract is not feasible or desired. In this variation:
CheckpointExecutor contract which provides methods to interact with the external firewall.SecurityValidatorThe SecurityValidator is a singleton contract where users save attestations for their transactions. These attestations include a deadline for validity and an ordered list of "execution hashes", ensuring that users execute checkpoints in the exact order approved by the attester. In other words, users must perform actions in the transaction that trigger security checks (checkpoints) in the same sequence as those considered by the attester during off-chain simulation.
Furthermore, the SecurityValidator is used by contracts implementing a Firewall to ensure that users provide valid attestations before executing actions deemed sensitive by the protocol (i.e., actions that trigger a checkpoint during execution).
To enable attesters to simulate transactions off-chain without needing attestations, the contract implements a bypass mechanism. This mechanism involves setting non-empty code at a specific address, which is easily achieved using cheat codes on a local fork of the chain, but impossible to do in actual on-chain execution.
AttestationForwarderThe AttestationForwarder contract is a forwarder compatible with ERC-2771 contracts, built on top of OpenZeppelin's ERC2771Forwarder. Its sole purpose is to sponsor calls that save attestations in persistent storage within the SecurityValidator contract, allowing users to load these attestations in subsequent transactions.
The Forta Attester architecture consists of a suite of contracts deployed on the Forta Chain, designed to manage roles, registries, controllers, and other components to ensure the system's decentralization and censorship resistance. The primary objectives of these contracts are to:
AttesterControllersThe AttesterControllers contract manages controllers within the system, each containing the encrypted configuration used by attesters and the delay duration for transactions flagged as high-risk. To foster decentralization, each protocol integrating with the Forta Firewall can deploy its own instance of this contract as the administrator role holds privileges over how attesters operate and are configured.
In addition, attesters use this contract to log detections as events. These events include a unique detection ID, a risk score, and the encrypted details of the detection. Detections surpassing a risk threshold are delayed and stored in the Inbox contract, a singleton that records release times for suspicious transactions. To ensure censorship resistance, once the release time passes, attesters are required to sign attestations for these transactions, even if initially flagged as high-risk.
As the Forta Network is divided into two main parts, the on-chain and the off-chain, each one has its own set of privileged roles.
DEFAULT_ADMIN_ROLE
FIREWALL_ADMIN_ROLE
PROTOCOL_ADMIN_ROLE
CHECKPOINT_MANAGER_ROLE
LOGIC_UPGRADER_ROLE
CHECKPOINT_EXECUTOR_ROLE
ATTESTER_MANAGER_ROLE
TRUSTED_ATTESTER_ROLE
SecurityValidator from the ERC2771Context contractstoreAttestation in the SecurityValidator contractDEFAULT_ADMIN_ROLE
FirewallRoleManager contract deployer initiallyCONTROLLER_SUPER_ADMIN_ROLE
CONTROLLER_CONFIG_ADMIN_ROLE
CONTROLLER_VIEWER_ROLE
UPGRADE_ADMIN_ROLE
AttesterControllers contractATTESTER_ROLE
AUTHENTICATOR_ROLE
The Forta Network's firewall and attestation system operates under several trust assumptions and security considerations:
AttesterControllers contract. Malicious upgrades could compromise the entire system.Users and developers interacting with the Forta Network's firewall and attestation system should be aware of the above trust assumptions and the potential consequences if they are violated. The security of the system heavily relies on the integrity of the privileged roles and the correct implementation of the various security mechanisms.
SecurityValidatorUsers interacting with contracts utilizing a Forta Firewall must provide an attestation that serves as proof that the transaction has been simulated and validated for non-malicious intent. These attestations can only be created by trusted attesters. The SecurityValidator contract allows these attesters to store attestations via the storeAttestation and saveAttestation functions. The former persists the attestation in storage until it is consumed, while the latter uses transient storage to conduct the attestation and execution within the same transaction.
During checkpoint executions, the getCurrentAttester function retrieves the current attester by checking transient storage and validating if the attester is trusted using _isTrustedAttester. However, if the getCurrentAttester function returns the zero address, the transaction does not revert, and the attester is then queried from the contract's storage in the executeCheckpoint function. The critical issue here is that the attester retrieved from storage is not validated as trusted, potentially allowing any entity to use storeAttestation to create fraudulent attestations that could enable malicious transactions to pass as valid.
Importantly, this vulnerability only manifests when the attestation contains a single executionHash. If the attestation includes multiple execution hashes, the transaction reverts. Despite this limitation, the issue remains critical as it still provides a potential avenue for bypassing the security checks which is the purpose of the firewall.
Consider implementing a validation check to ensure that any attester retrieved from storage is indeed trusted, preventing unauthorized entities from bypassing the security mechanism.
Update: Resolved in pull request #9.
Attestations include an ordered list of execution hashes representing the sequence of checkpoints to be executed. These execution hashes are derived from checkpoint hashes, constructed by hashing the sender's address, the firewall's address, the function selector, and the quantized reference argument. However, this information alone is insufficient to determine whether a function call poses a risk to the protocol. Specifically:
For the reasons mentioned above, an attacker could request an attestation for harmless function calls that do not represent their true intent for on-chain execution. After obtaining the signed attestation, they could craft a malicious transaction by modifying any non-reference arguments and adding additional function calls that change the state without triggering checkpoints. This allows them to use the original signed attestation to execute the malicious transaction.
Consider providing additional context within attestations, allowing attesters to perform analyses in an environment that more accurately mimics actual on-chain execution. This approach would help prevent attackers from submitting modified transactions that produce outcomes significantly different from those attested.
Update: Partially resolved in pull request #10 and pull request #30. Attestations still do not account for additional function calls that could modify the state without triggering checkpoints. The Forta team stated:
Added support for byte range configuration larger than 32 to enable usage of more bytes from call data as the checkpoint hash input. Added more support for large call data ranges with a second PR. We will not support stateful checkpoint hash computation in the library since it can be very challenging to generalize but it will be possible for the user/developer to support that easily by, e.g., getting the caller's asset balance from a mapping and using it in the checkpoint hash computation.
Contracts using Firewall-based contracts require attestations from trusted entities (Attesters) to ensure that the transactions are not malicious. If an attestation is not present, the transaction will revert. Currently, users can only obtain these attestations through the Dapp's front-end, which forwards the intended transaction to the attester before constructing a transaction for the user to sign, either with or without an embedded attestation, depending on how it is stored.
This design restricts composability as users cannot perform complex interactions outside the front-end’s capabilities, such as multi-contract operations (e.g., arbitrage), due to the inability to obtain the necessary attestation. This limitation may deter users from using the Dapp and prevent its integration with other Dapps.
Consider implementing a mechanism that allows users to obtain attestations without relying solely on the Dapp's front-end.
Update: Acknowledged, not resolved. The Forta team stated:
The
SecurityValidatorcontract and Firewall implementations do not suggest how attestations can be created and delivered off the chain. While dApp integration is one way to deliver them, there are other ways to deliver such as writing a JSON-RPC proxy to do front running, building a custom API to do transaction forwarding, or constructing EIP-7702 transactions in the near future.Any contract that depends on off-chain services needs to have this kind of flow if the contract does not implement a request-response base like Chainlink Functions, which we have not been in favor of and consider to be more complex from other angles. Multi-contract operations are feasible since an attestation contains all checkpoint execution hashes that will be produced by the
executeCheckpoint()call of any integrator in the same transaction. Delivering the attestation in the same transaction brings some friction since it requires a function likeattestedCall()to first save the attestation in the first contract call. We have developed the two-transaction pattern to let a proxy front-run by saving an attestation first before forwarding the user transaction. And later, when introduced, EIP-7702 will allow enabling the trusted origin pattern implemented in the Firewall, which bypasses the checkpoint execution in case thetx.originis trusted - saving attestations might not be necessary in most cases if this approach gets adopted.However, regardless of the above, it is hard to deny that contract-to-contract integrations can be impacted from the contract's side which does not integrate Forta Firewall. We have been aware of this issue since the initial phases of this design and are actively thinking of ways to reduce this impact. This is unfortunately not a kind of problem we can solve as part of this audit.
SecurityValidatorThe SecurityValidator contract relies on attestations, which are essentially ordered arrays of hashes generated during each checkpoint execution. These attestations should validate the entire execution path, guiding the attester through a specific sequence of actions.
However, the current implementation allows attestations to be partially executed which contradicts the intended purpose that they should only be valid if the entire execution path is followed. This flaw could be exploited by malicious users. For instance, an attacker could execute a transaction designed to exploit a protocol but include a final step that returns funds to the protocol, misleading Forta validation mechanisms into believing that the transaction is non-malicious. Once users have the signed attestation, they can submit the transaction on-chain while omitting the final return step, allowing them to steal funds by bypassing the security check.
Consider enforcing stricter validation of the attestation process to ensure that all checkpoints are properly executed. For example, consider incorporating a mandatory call to the validateFinalState method to confirm that the entire attestation process has been completed as expected.
Update: Acknowledged, not resolved. The Forta team stated:
We acknowledge that it is possible to save an attestation that contains hashes A, B, and C, then just to A and B, and exit the transaction successfully. Euler's EVC can schedule a final
validateFinalState()call since it sits in front of every call to every vault in the same transaction if the user uses an attestation in theEVC.batch()call. In the case of a different consumer:
- The integrated contract needs to receive the user call through a trusted contract and do something similar to
_msgSender()to deduce the actual account to operate on. This brings more integration friction and reduces the chances of Forta Firewall adoption.- We can put
validateFinalState()at the end ofattestedCall()but if any contract uses OpenZeppelin Multicall, then a user can elect to send an attested call that does not finally validate.- If the attestation is delivered from a different integrator, then another integrator that is in the middle of the call stack can have no guarantees that the first contract in the call stack will do
validateFinalState(). Currently, this seems hard to solve but we are not too worried about this issue.For state and code at a given time/block range, we expect C to be consumed if A and B are consumed. If the state or code changes then the checkpoint hashes should also change. We are doing our best to ensure this in the Firewall base implementation. We will try to deliver improvements to the Firewall implementation in the next iterations and are giving every user the freedom to write their own firewall and checkpoints, with or without the Forta Firewall contract library.
Attestations include an ordered list of execution hashes, representing the sequence of checkpoints to be executed. For each checkpoint, a hash is constructed by hashing the sender's address, the firewall's address, the function selector, and the quantized reference argument. Attesters rely on these checkpoint hashes, meaning their analyses only consider the quantized reference argument instead of the actual value that will be used on-chain. There are two scenarios where the attested value of the reference argument may differ significantly from the actual value provided by the user during on-chain execution:
quantize function performs downward quantization.AlwaysActive activation mode is used for a checkpoint.On the one hand, the quantize function in the Quantization library zeroes out the least significant bits of a given value. In the worst case, it converts values of the form 2 * 256^n - 1 into 256^n, resulting in a precision loss of nearly a factor of two. An attacker could exploit this behavior by obtaining valid attestations for transactions that are dangerous to the protocol, as the attester is only evaluating half of the real reference value that will be sent on-chain. For instance, if the reference argument represents a withdrawal amount, an attacker could request an attestation to withdraw their entire balance, which the attester would validate. However, when submitting the actual on-chain transaction, the attacker could double the withdrawal amount, exceeding their balance while still using the original attestation due to the quantized values being checked.
On the other hand, when the AlwaysActive activation mode is set for a checkpoint, the reference value is replaced with the constant 1 to construct attestations. Consequently, attestations for this checkpoint will permit execution with any reference argument provided by the user when interacting with the protocol, as the attester always sees the constant value 1, regardless of the actual value supplied. As a result, attackers have even more freedom than in the previous case to alter the reference argument with respect to the attested value.
Consider performing upward quantization instead of downward to ensure that attesters' analyses favor the protocol. In addition, consider using the actual reference argument instead of replacing it with a constant when the checkpoint activation mode is set to AlwaysActive.
Update: Resolved in pull request #14.
Currently, the system does not offer users a method to challenge delays imposed on transactions. If a valid transaction is mistakenly flagged as harmful, and a long delay is set by an attester, users are subjected to extended waiting times with no recourse to dispute the delay. Moreover, a malicious attester could use the logDetection function to arbitrarily delay transactions, potentially causing actions such as forcing liquidations in lending protocols. Legitimate attesters have no mechanism to revoke such delays, increasing the risk of exploitation.
Consider introducing a process to allow users to contest transaction delays and enabling legitimate attesters. This could involve a proof submission system for valid transactions or a more structured review process for disputed delays. Doing this will help remove unjustified delays and mitigate the risks
Update: Resolved in pull request #21. Admins can now update the release time for transactions that were incorrectly delayed.
Inbox and AttesterControllers ContractsBoth the Inbox and AttesterControllers contracts allow for delaying transactions. However, they do so in inconsistent ways which could lead to security, auditability, and efficiency issues.
In the Inbox contract, the delayFFR function allows attesters to delay transactions with minimal validation. In contrast, the logDetection function within AttesterControllers enforces stricter checks, such as risk logging and the application of predefined delay durations.
This inconsistency carries several issues:
delayFFR function allows arbitrary future timestamps which may override controller settings and result in transactions being delayed indefinitely.logDetection, undermining the integrity of the delay system.delayFFR reduces transparency, making it difficult to trace the rationale for transaction delays.logDetection function calls delayFFR, which redundantly checks whether the caller is an attester, even though this validation has already been performed. Furthermore, the logging of both the FFRDelayed and TxDelayed events in the same transaction could be redundant.To address the aforementioned issues, consider making the logDetection function the sole entry point for delaying transactions to ensure consistent handling of delays. In addition, removing redundant identity checks and enhancing logging would improve the system’s security, efficiency, and auditability.
Update: Partially resolved in pull request #21. Compared to using logDetection, delaying transactions via delayFFR directly still omits logging detection details and bypasses the delay durations configured for each controller in the AttesterControllers contract. The team stated:
The logging of both the FFRDelayed and TxDelayed events in the same transaction is not redundant, because delayFFR on inbox can be called by different AttesterControllers instances. So
FFRDelayedevent on AttesterControllers is required to extend data about which controller ids was affected by this delay. Inbox doesn't care about controllers just about FFRs and their delays.
The system allows extending the release time of a transaction through multiple invocations but prevents shortening the delay. Specifically, by repeatedly calling the delayFFR function in the Inbox contract or the logDetection function in the AttesterControllers contract with the same data, an attester can continually push back the release time. This creates a risk of transaction censorship, as an attester could indefinitely delay the processing of a transaction.
To mitigate this risk, consider limiting the number of times the release time for a given transaction can be updated. Alternatively, consider imposing a maximum delay period to ensure that the release time cannot be extended indefinitely.
Update: Resolved in pull request #21. Admins can now update the release time for transactions that were incorrectly delayed, and a maximum delay duration has been introduced.
The (Forta Firewall and Forta Attester Controller) codebases exhibit insufficient test coverage for branches and functions (currently under 60%). This level of coverage may leave critical portions of the code untested, potentially leading to undetected vulnerabilities.
Consider adding more unit tests to increase the test coverage to above 95%, adhering to the best practices for software security and reliability. In addition, consider integrating test coverage tracking into the repository's Continuous Integration process for ongoing quality assurance.
Update: Acknowledged, not resolved.
SecurityValidator Does Not Inherit From ISecurityValidatorThe SecurityValidator contract does not inherit from the ISecurityValidator interface, which could lead to the interface and its implementation going out of sync.
Consider making the SecurityValidator contract inherit from the ISecurityValidator interface.
Update: Resolved in pull request #4.
To execute checkpoints in the SecurityValidator contract, attestations can either be saved directly to transient storage for use within the same transaction or saved into persistent storage for use in future transactions. However, while the first method allows new attestations to be saved after previous attestations have been used, the second method does not. As a result, if a user wanted to start a transaction by loading attestations with saveAttestation, then consume those attestations, and then load more attestations from storage to keep executing checkpoints (which must have been loaded in a previous transaction using storeAttestation), they would not be able to do so.
Consider allowing attestations to be loaded from persistent storage even after previous attestations have been used, ensuring consistency between both methods of saving attestations.
Update: Resolved in pull request #3. The Forta team stated:
Applied a fix to support stored attestations after saved attestations are consumed.
Several contracts in the codebase implement public functions that are not part of their interface. As a result, third parties interacting with these contracts via their interface will be unable to call those functions. In particular:
ISecurityValidator interface does not include the validateFinalState and executionHashFrom functions.IFirewall interface does not include the attestedCall function.Consider adding the missing functions to the interfaces to make it easier for third parties to interact with the contracts.
Update: Resolved in pull request #4.
require StatementsIn FirewallPermissions.sol, all modifiers are missing error messages in their require statements.
Consider including specific, informative error messages in require statements to improve overall code clarity and facilitate troubleshooting whenever a requirement is not satisfied.
Update: Resolved in pull request #15.
In AttesterControllers.sol, multiple instances of incomplete docstrings were identified:
ffrIds parameter of the StoreBatch event is not documented.encryptedDetails parameter of the Detection event is not documented.configuration parameter of the registerController function is not documented.logExceedsThreshold and delayExceedsThreshold parameters of the logDetection function are not documented.Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved at commit 717626f.
Throughout the codebase, multiple instances of missing docstrings were identified. Some notable examples include:
Whitelist.sol, the addToWhitelist functionInbox.sol, the TxDelayed eventAttesterControllers.sol, the grantSuperAdminRole functionFirewall.sol, the SecurityConfigUpdated eventFirewall.sol, the SupportsTrustedOrigin eventConsider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved at commit 717626f.
When assigning an address from a user-provided parameter, it is crucial to ensure that the address is not set to zero. Setting an address to zero is problematic because this action has special renounce semantics.
Throughout the codebase, multiple instances of assignment operations missing zero address checks were identified:
firewallAccess assignment operation within the contract FirewallPermissions contractnewImplementation assignment operation within the ProxyFirewall contractroleManagerAddress assignment operation within the AttesterControllers contractinboxAddress assignment operation within the AttesterControllers contractConsider adding a zero address check before assigning a value to a state variable.
Update: Resolved in pull requests #18 and #16.
The ProxyFirewall contract contains a receive fallback that allows users to send ETH to the contract. However, it lacks a withdrawal mechanism, creating a potential risk of permanently locking any ETH sent to the contract. While accidental ETH transfers to this contract are unlikely, the possibility still exists.
Consider removing the receive fallback unless the functionality of sending ETH to the ProxyFirewall contract is needed. Alternatively, implement a mechanism to allow the withdrawal of the received ETH.
Update: Resolved in pull request #18. The Forta team stated:
We want the proxy firewall to be plugged in as an intermediary logic contract between, e.g., an ERC1967 Proxy and a logic contract. We added receive function to suppress Solidity warnings before and now corrected that to use the fallback logic. However, we realized that the checkpoint configuration does not know how to work with ether amounts in
msg.value. So, we adapted the configuration logic to takemsg.valueas the reference value whenmsg.sigor ref range is zero.
DEFAULT_ADMIN_ROLEThe FirewallAccess contract implements access control. The DEFAULT_ADMIN_ROLE role is given to the default admin address when deploying the contract. The admin role of the FIREWALL_ADMIN_ROLE role is the DEFAULT_ADMIN_ROLE role as it is not explicitly assigned in the code. Since this role has special privileges associated with it, such as the ability to grant or revoke the FIREWALL_ADMIN_ROLE role or the DEFAULT_ADMIN_ROLE role many times, further security measures should be taken.
According to the comments in OpenZeppelin's AccessControl.sol file as well as the official documentation, the recommendation is to use the AccessControlDefaultAdminRules contract. This contract implements the following risk mitigations on top of AccessControl:
DEFAULT_ADMIN_ROLE role since deployment until it is potentially renounced.DEFAULT_ADMIN_ROLE role to another account.DEFAULT_ADMIN_ROLE role.Consider using AccessControlDefaultAdminRules in order to safely manage the DEFAULT_ADMIN_ROLE role.
Update: Acknowledged, not resolved. The Forta team stated:
We do not feel that granting or revoking top-level roles many times is a problem. So, it feels like
AccessControlDefaultAdminRulesmight add needless complexity to the overall firewall management experience.
The addToWhitelist and removeFromWhitelist functions of the Whitelist contract emit events even when there is no actual change to the whitelist. For instance, if removeFromWhitelist is called with a value that is not in the whitelist, it still emits the WhitelistReduced event, which is misleading because the whitelist remains unchanged. Similarly, addToWhitelist emits the WhitelistExtended event even when the value is already whitelisted.
Consider adding a condition to only emit events when the whitelist is effectively modified. This will ensure that the WhitelistReduced and WhitelistExtended events are only triggered when an address is truly added to or removed from the whitelist, avoiding potential confusion in event logs and monitoring.
Update: Resolved at commit 0d5c3d0.
The system currently does not impose a maximum limit on the delay duration for transactions flagged as suspicious. This lack of a constraint allows any actor with the DEFAULT_ADMIN_ROLE, controller admins, or an attester calling delayFFR directly, to set arbitrary delay values without restriction.
Consider introducing a maximum delay (e.g., MAX_DELAY). This would help mitigate potential abuses, such as censorship or DoS scenarios, by limiting the extent to which delays can be manipulated.
Update: Resolved at commit c944625.
Multicall functionality is added to all proxy variants to enable saving attestations and executing protocol logic in the same transaction. While effective for the InternalFirewall, this approach is less suited for the ExternalFirewall and ProxyFirewall contracts.
Considering the ExternalFirewall contract, since it does not contain protocol logic, the multicall feature will not serve its main purpose. In this case, the primary benefit of this feature is the ability to configure multiple checkpoints in one transaction. However, this introduces inefficiencies due to unnecessary delegate calls. A more optimized solution would be a custom batch function to set checkpoints, reducing redundant calls and gas costs.
In the case of the ProxyFirewall, using multicall leads to significant gas inefficiencies as delegatecalls from the ProxyFirewall target the front proxy instead of the ProxyFirewall itself. This results in extra delegatecalls being triggered, which is inefficient and introduces potential risks. A better approach would be to implement the multicall functionality in the front proxy, avoiding unnecessary calls and maintaining the intended security model.
Consider removing the multicall feature from the ExternalFirewall and ProxyFirewall contracts and replacing it with a solution tailored to their specific use cases.
Update: Resolved in pull request #17.
The storeAttestation function in the SecurityValidator contract allows users to save attestations in persistent storage for use in future transactions. These attestations are saved in a mapping, in which the key corresponds to the sender of the transaction given by the _msgSender function. However, when attestations are finally loaded from persistent storage to transient storage, the tx.origin address is used to access the mapping.
When users interact with the protocol using any type of smart contract account (such as Safe, ERC-4337 smart accounts, etc.) instead of EOAs, and call the storeAttestation function, the attestations are saved in the mapping under the smart account's address. However, when the users submit their transactions and the executeCheckpoint function is called, the protocol searches for the necessary attestations in the mapping under the tx.origin address, which corresponds to the EOA that initiated the transaction, not the users' smart contract accounts. As a result, the attestations are not found and the transaction reverts.
Consider avoiding the use of tx.origin to access attestations in the mapping, thereby allowing smart contract account users to interact with the protocol in the same way as EOA users.
Update: Acknowledged, not resolved. The Forta team stated:
The reason for using
tx.originis to avoid weird collisions across users when their EOAs need to use attestations that start with the same execution hash. ERC-4337 supports batching thanks to bundlers so any attestation should ideally be provided as part of the sameUserOperationbundle, in the same tx. The use of dual transaction (attestation tx + user tx) is not considered as a use case for ERC-4337 smart accounts and we consider this issue to be safe to ignore.
The inclusion of additional functions in the Firewall contract to handle function signatures aims to enhance the developer experience. However, it significantly increases the contract's size without providing substantial value. This increase in size may also reduce the margin available for contracts that inherit from it, potentially bringing the contract closer to the 24576 B limit imposed by EIP-170.
To maintain a more streamlined and elemental implementation, consider only retaining the methods that work with selectors directly.
Update: Resolved in pull request #11.
Throughout the codebase, multiple opportunities for improving code quality were identified:
delete keyword is being used as if it were a function even though it is not. Consider removing the parentheses and adding a space between the keyword and the mapping._tryInitAttestationFromStorage function should return false instead of reverting when no stored attestation is found. This would be to maintain consistency with the rest of the function logic and its return type.Consider incorporating these changes to enhance code clarity and readability.
Update: Resolved in pull request #5. The Forta team stated:
We addressed the second issue in the fix for L-03.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Throughout the codebase, multiple instances of mappings without named parameters were identified:
checkpoints state variable of the Firewall contractattestations state variable of the SecurityValidator contractwhitelist state variable in the Whitelist contractConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #6 and at commit 180a7cb.
The use of non-explicit imports in the codebase can decrease code clarity and may create naming conflicts between locally-defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.
Within the codebase, multiple instances of global imports were identified:
Following the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
Update: Resolved in pull request #7 and at commit 8c38c9d.
Throughout the codebase, multiple instances of state variables lacking an explicitly declared visibility were identified:
In Inbox.sol:
In Registry.sol:
In SecurityValidator.sol:
For improved code clarity, consider always explicitly declaring the visibility of state variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #8 and at commit 95e2a94.
Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as that function's output. They are an alternative to explicit in-line return statements.
Firewall.sol, all the named return variables of the getFirewallConfig function are unused.AttesterControllers.sol, the controllerId return variable of the registerController function is unnecessarily returned.Consider either using or removing any unused named return variables and eliminating unnecessary explicit return statements.
Update: Resolved in pull request #17. Line 185 was removed by mistake in the PR, but later reverted.
Throughout the codebase, multiple opportunities for optimizing gas usage were identified:
Loop Optimizations:
i++) with the pre-increment (++i) operator to save gas where the return value of the expression is not used:
executionHashes within the _initAttestation functioncontrollerIds within the logDetection functiontargetContracts[entityId] within the removeTargetContract function (also accessed within the loop body)Custom Errors:
Hashing Optimizations:
Registry.sol, the _targetContract is rehashed on every iteration instead of reusing the already computed hash.Optimizable State Reads:
Firewall.sol: Change the checkpoint parameter in _checkpointActivated and the checkpoint variables in getCheckpoint, and in both versions of _secureExecution (line 309 and line 317) from storage to memory. Neither function modifies the checkpoint, so they can cache and reference it from memory instead of accessing it from storage.Inbox.sol: The inboxReleaseTimestamps mapping in the delayFFR function is being accessed twice for the same key.AttesterControllers.sol: The _controllersIdCounter state variable in the registerController function is being read from storage twice: once implicitly, when its value is updated, and once for returning it.Update: Partially resolved in pull request #27.
Throughout the codebase, multiple instances of redundant code were identified:
getControllerById function is redundant because the public controllers state variable already has an automatically included getter.msgSender local variable in the storeAttestation function is only used once, so it could be removed in favor of a direct call to the _msgSender() function.delayFFR function is only used once, so it could be in-lined within logDetection to reduce contract size and avoid a redundant call to the onlyAttester modifier.Consider eliminating the redundant code to enhance readability and efficiency.
Update: Acknowledged, not resolved.
Throughout the codebase, multiple instances of misleading documentation were identified:
SecurityValidator contract is described as being intended for attesters and contracts. However, it is more accurate to say that it is meant for attesters and firewalls.upgradeToAndCall and the UUPSUpgradeable contract implements upgradeNextAndCall, when in fact, it is the other way around.@notice comment that contains the firewall description has an incomplete sentence._updateFirewallConfig function initializes the firewall configuration for the first time. However, this function is called every time the configuration is updated, not just the first time.delayFFR function incorrectly describes it as an event.ExternalFirewall NatSpec is outdated: the withCheckpoint modifier no longer exists, and the executeCheckpoint and setCheckpoint functions now receive additional arguments.InternalFirewall and ProxyFirewall NatSpecs are outdated: the _secureExecution and setCheckpoint functions now receive additional arguments.Consider correcting the aforementioned comments to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #26.
Throughout the codebase, an instance of a typographical error was identified:
ProxyFirewall.sol, the word "point" is repeated at the start of the next line.Consider fixing the typographical error to improve the readability of the codebase.
Update: Resolved in pull request #23.
The RoleManager.sol file name does not match the FirewallRoleManager contract name.
To make the codebase easier to understand for developers and reviewers, consider renaming the file names to match the contract names.
Update: Resolved at commit 99231f9.
Most contracts use Solidity ^0.8.25 floating pragma. Using a fixed pragma for other contracts ensures consistent compilation and behavior across environments, while maintaining flexibility where needed.
Consider using fixed pragma (e.g., pragma solidity 0.8.25;) for all contracts except InternalFirewall and CheckpointExecutor. These two should keep floating pragma as they have been designed to be inherited, enhancing integration flexibility and developer experience.
Update: Acknowledged, not resolved.
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the codebase, none of the contracts include a security contact.
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Acknowledged, not resolved.
Several files in the codebase, such as SecurityValidator.sol and Firewall.sol, contain both interfaces and contract definitions. This practice can reduce modularity and clarity as it mixes contract logic with interface definitions, which are conceptually distinct.
Consider moving each interface into its own dedicated file. This separation enhances maintainability, allows interfaces to be referenced more easily, and aligns with best practices for clean code architecture. Moreover, consider relocating the NatSpec comments from the functions in the contracts to the corresponding interfaces and using the @inheritdoc tag in the contracts to inherit these comments. NatSpec comments are more beneficial in interfaces since developers interacting with the contracts typically rely on them for reference.
Update: Resolved in pull request #29. The team stated:
I rearranged interfaces for easier and more granular import. I generally agree with the suggestion related to
@inheritdocbut I don't find it particularly useful in this case. I think that the current comments are more valuable when they sit next to the implementation. I think that this practice might be more useful for ERC interfaces that have wider reuse.
Throughout the codebase, multiple instances of inaccurately named errors and state variables declarations were identified:
InvalidThresholdType: This error name is not entirely accurate and could be misleading. The error is thrown when the checkpoint.activation is not one of the expected Activation types. It is not specifically about an invalid threshold type, but rather an invalid or unexpected activation type. A more accurate name could be InvalidActivationType or UnexpectedActivationType.Checkpoint: The term "checkpoint" is overloaded in the codebase, leading to potential confusion. While the Checkpoint struct in the Firewall contract represents configuration settings for security checks, the executeCheckpoint function in SecurityValidator performs runtime verification using a hash that is unrelated to the Checkpoint struct. Renaming the struct to CheckpointConfig may better distinguish it from the checkpoint hash used for verification.Consider addressing the aforementioned naming issues to improve the readability of the codebase.
Update: Partially resolved in pull request #25. The team stated:
Agree about the first one.
The use of // SPDX-License-Identifier: UNLICENSED in the code indicates that no explicit license has been specified. This has significant implications:
Restricted Usage by Default: Without a license, others are not granted permission to copy, modify, or distribute the code. Copyright laws automatically apply, protecting the code, and any use without permission could be considered infringement.
Enforceability Concerns: Even though the code is legally protected, the absence of a clear license might make it harder to enforce these rights. Some users may mistakenly believe that the code is open for use due to the lack of a license statement.
No Attribution Requirements: Since no license is provided, there are no conditions like attribution requirements. However, others are not permitted to use the code at all without explicit permission from the author.
To ensure control over how your code is used, consider specifying a license. A license provides legal protection by establishing clear usage terms which helps prevent misuse and encourages collaboration.
Update: Resolved in pull request #24.
AccessControl in the Whitelist ContractThe Whitelist contract currently only uses the DEFAULT_ADMIN_ROLE role. If the contract intends to have only one role and one account managing this role, consider switching to Ownable to simplify the implementation. Using AccessControl for a single role can be unnecessarily complex and inefficient.
In addition, for enhanced security, consider implementing Ownable2Step. This would help prevent potential errors during ownership transfers, such as transferring ownership to an incorrect address or to contracts that cannot interact with the permission system.
Update: Acknowledged, not resolved.
Throughout the codebase, multiple instances of contracts having inconsistent ordering of functions were identified:
ProxyFirewall contract in ProxyFirewall.solAttesterControllers contract in AttesterControllers.solRegistry contract in Registry.solTo improve the project's overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide (Order of Functions).
Update: Resolved at commit 0f7bb68.
Throughout the codebase, multiple instances of events not having indexed parameters were identified:
In Firewall.sol:
In Registry.sol:
To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Update: Resolved in pull request #22 and at commit 6879e15. The Forta team stated:
Fixed for
Registry.sol.
Throughout the codebase, multiple instances of unused errors were identified:
Firewall.sol, the AlreadyInitialized error is unused.Errors.sol, none of the errors are used.To improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused errors.
Update: Resolved in pull request #21, and commit dd9fd18. The Forta team stated:
Fixed for
Errors.sol. Removed unused errors.
Throughout the codebase, multiple instances of unused imports were identified:
import {ISecurityValidator, Attestation} from "./SecurityValidator.sol"; imports unused alias Attestation in ExternalFirewall.sol.import {ISecurityValidator, Attestation} from "./SecurityValidator.sol"; imports unused alias Attestation in InternalFirewall.sol.import {ISecurityValidator, Attestation} from "./SecurityValidator.sol"; imports unused alias Attestation in ProxyFirewall.sol.import "./Errors.sol"; imports unused errors in AttesterControllers.sol.import "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; imports unused library SignatureChecker in SecurityValidator.sol.import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol"; imports unused interface IAccessControl in FirewallConsider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #20.
Firewall Does Not Need To Be InitializableThe Firewall contract inherits from the Initializable contract, despite not using any of its functionality. Furthermore, the only firewall variant that requires initialization is ProxyFirewall.
To avoid unnecessarily increasing contract size, consider limiting the Initializable inheritance exclusively to the ProxyFirewall contract.
Update: Resolved in pull request #19.
When integrating with the firewall, DeFi protocols are allowed to define their own set of trusted attesters. However, all integrators are expected to use the default attester set managed by the Forta team unless explicitly configured otherwise. If two protocols interact within the same transaction and rely on mutually exclusive attester sets, this may result in a composability issue.
Consider adding documentation to explain this situation, and ensure that all protocols integrating with the Firewall include Forta's attesters in their list of trusted attesters.
Update: Acknowledged, will resolve.
The Forta Firewall's checkpoint mechanism uses fixed refStart and refEnd fields to locate reference values in transaction calldata. This approach has limitations for functions with variable-length arguments (e.g., arrays), leading to several issues:
Consider adding support for functions with variable-length arguments in later iterations of the firewall.
Update: Acknowledged, will resolve.
To prevent replayability, attestations include a validity deadline. However, users can still replay a transaction using the same attestation repeatedly, as long as it occurs before the deadline.
This opens the door for a malicious user to break down an attack into smaller, seemingly harmless steps. Once they receive an attestation for executing a single step, they can reuse it to execute the entire attack by resending the transaction multiple times.
This issue is currently deemed out of scope to prioritize gas efficiency, but potential solutions are being explored. Consider adding a nonce to the attestations so that they can only be used once.
Update: Acknowledged, not resolved.
The Forta Firewall is an innovative security solution designed to enhance the protection of DeFi protocols. This system allows for the screening of transactions before they are sent to the blockchain, using a checkpoint mechanism and security attestations to prevent potentially malicious actions.
During our audit, we found one critical- and one high-severity issues. While these findings are concerning, the overall design of the Firewall is robust. We were really pleased with how the Forta team handled the audit process. They were quick to respond, eager to discuss our findings, and showed a genuine commitment to making their product as secure as possible. Despite the issues we found, we are optimistic about the Forta Firewall's potential as it could be a game-changer for DeFi security.