Skip to content

Forta Firewall Audit

 

Table of Contents

Summary

Type
Security Infrastructure
Timeline
From 2024-09-09
To 2024-09-23
Languages
Solidity
Total Issues
47 (31 resolved, 4 partially resolved)
Critical Severity Issues
1 (1 resolved)
High Severity Issues
1 (0 resolved, 1 partially resolved)
Medium Severity Issues
6 (3 resolved, 1 partially resolved)
Low Severity Issues
14 (11 resolved)
Notes & Additional Information
22 (16 resolved, 2 partially resolved)
Client Reported Issues
3 (0 resolved)

Scope

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

System Overview

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:

  • Before the transaction execution, using the storeAttestation() function, which utilizes SSTORE to persist the attestation in contract storage.
  • At the beginning of the same transaction, using the 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.

Firewall

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:

  • Checkpoint configuration and management
  • Secure execution flow control
  • Integration with external security validators to save attestations and execute checkpoints

Quantization

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.

Firewall Variations

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.

Proxy Firewall

The Proxy Firewall is an approach that leverages the proxy pattern common in upgradeable smart contracts. Its characteristics include:

  • It acts as an intermediary contract sitting between a proxy contract and the logic contract.
  • The proxy firewall intercepts all calls to the logic contract, executes necessary security checks, and then delegates the call to the actual logic contract.
  • This variation requires no modifications to the existing logic contract, making it ideal for protocols using upgradeable proxy patterns.
  • It adds a small gas overhead to each transaction but offers seamless integration of security features.

Internal Firewall

The Internal Firewall is suitable for protocols that can directly incorporate firewall logic into their contracts. Key features include:

  • Firewall functionality is inherited directly by the target contract.
  • Checkpoint configurations are stored within the contract itself.
  • This variation offers tighter integration and potentially lower gas costs for checkpoint executions.
  • It requires more significant modifications to the existing contract but provides greater control over the firewall's operation.

External Firewall

The External Firewall is designed for scenarios where direct modification of the target contract is not feasible or desired. In this variation:

  • The firewall logic is implemented in a separate contract.
  • The target contract inherits from a CheckpointExecutor contract which provides methods to interact with the external firewall.
  • Checkpoint configurations and executions are managed by the external firewall contract.
  • This approach allows for minimal changes to the existing contract while still providing robust security features.

SecurityValidator

The 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.

AttestationForwarder

The 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.

Forta Chain Contracts

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:

  • Act as a public, immutable ledger for attester detections.
  • Store encrypted per-protocol configurations for the attester client.
  • Serve as a global database for transactions that have been deemed insecure and delayed.

AttesterControllers

The 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.

Privileged Roles

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.

On-Chain Firewall System Roles

  1. DEFAULT_ADMIN_ROLE
    • Can grant and revoke the Firewall Admin role
    • Assigned at construction time
  2. FIREWALL_ADMIN_ROLE
    • Highest level of access in the firewall system
    • Can manage Protocol Admins
    • Responsible for overall firewall configuration and management
  3. PROTOCOL_ADMIN_ROLE
    • Can manage Checkpoint Managers, Logic Upgraders, Checkpoint Executors, and Attester Managers
  4. CHECKPOINT_MANAGER_ROLE
    • Can set and modify checkpoints in the firewall system
  5. LOGIC_UPGRADER_ROLE
    • Can upgrade the logic of a specific proxy firewall
  6. CHECKPOINT_EXECUTOR_ROLE
    • Can execute checkpoints in external firewall architectures
    • Typically the smart contract that is going to be protected by the firewall
  7. ATTESTER_MANAGER_ROLE
    • Can manage Trusted Attesters
  8. TRUSTED_ATTESTER_ROLE
    • Can provide security attestations for transactions
    • Responsible for validating and approving transactions based on security criteria
  9. Trusted Forwarder
    • Inherited by the SecurityValidator from the ERC2771Context contract
    • Can forward meta-transactions on behalf of the users to call storeAttestation in the SecurityValidator contract

Off-Chain Roles (Forta Chain)

  1. DEFAULT_ADMIN_ROLE
    • Can grant and revoke roles like Attesters, Authenticators, and Super Admins
    • Can register controllers
    • Assigned to the FirewallRoleManager contract deployer initially
  2. CONTROLLER_SUPER_ADMIN_ROLE
    • Can manage Controller Config Admin and Controller Viewer roles
    • Can update Controllers fields
  3. CONTROLLER_CONFIG_ADMIN_ROLE
    • Can update Controllers fields
  4. CONTROLLER_VIEWER_ROLE
    • Have read-only access to controller information
  5. UPGRADE_ADMIN_ROLE
    • Can upgrade the AttesterControllers contract
    • Responsible for managing contract upgrades
  6. ATTESTER_ROLE
    • Can log detections, store batches, and delay transactions
  7. AUTHENTICATOR_ROLE
    • Can add or remove target contracts from controller entities

Security Model and Trust Assumptions

The Forta Network's firewall and attestation system operates under several trust assumptions and security considerations:

  1. Role Integrity: All addresses with privileged roles are assumed to be trustworthy and not compromised. If compromised, these roles could significantly impact the system's security and potentially users' funds.
  2. Attestation Validity: The system relies on Trusted Attesters to provide valid and accurate security attestations for transactions. It is assumed that these attesters are not compromised and act honestly.
  3. Attestation Censorship: Transactions that have been delayed must be executable once the release time passes. It is assumed that attesters will generate attestations for these transactions immediately, without further analysis.
  4. Controller Management: Controller Super Admins and Config Admins are trusted to manage their controllers responsibly. Mismanagement on configurations and delay durations as well as incorrect public keys could lead to security vulnerabilities or system malfunctions.
  5. Upgrade Security: Upgrade Admins are assumed to only authorize legitimate and secure upgrades to the AttesterControllers contract. Malicious upgrades could compromise the entire system.
  6. Checkpoint Integrity: Checkpoint Managers are trusted to set appropriate and secure checkpoints. Incorrectly configured checkpoints could lead to security vulnerabilities like denial of services.
  7. Firewall Configuration: Firewall Admins and Protocol Admins are trusted to maintain secure and appropriate configurations. Misconfiguration could lead to system-wide vulnerabilities.
  8. Bypass Mechanism: The system includes a bypass mechanism for attestation requirements. It is assumed that this bypass mechanism will only work off-chain.
  9. Attestation Timing: The off-chain attestation process takes time which can be crucial for certain protocol operations, particularly those involving integrations or token exchanges. It is expected that protocols will consider this delay, assess the associated risks, and implement measures to mitigate them as much as possible.

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.

 

Critical Severity

Attestation Validation Bypass in SecurityValidator

Users 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.

High Severity

Attestations Lack Sufficient Information for Accurate Verification

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:

  • Attestations lack the context of what the other arguments to the function are and only consider the reference argument.
  • Attestations do not consider additional function calls that might modify the state without triggering checkpoints.

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.

Medium Severity

Limited Composability Due to Front-End Dependent Attestation Process

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 SecurityValidator contract 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 like attestedCall() 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 the tx.origin is 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.

Incomplete Validation of Attestations in SecurityValidator

The 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 the EVC.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 of attestedCall() 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.

Attested Values May Significantly Differ From On-Chain Execution

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:

  • When the quantize function performs downward quantization.
  • When the 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.

Lack of Challenge Mechanism for Incorrect Transaction Delays

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.

Inconsistent Delay Mechanisms in Inbox and AttesterControllers Contracts

Both 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:

  • Fragmented Logic: The different delay mechanisms create fragmented logic, leading to inconsistent behavior between contracts.
  • Potential Transaction Censorship: The delayFFR function allows arbitrary future timestamps which may override controller settings and result in transactions being delayed indefinitely.
  • Bypass of Controls: Attesters can bypass the stricter delay controls enforced by logDetection, undermining the integrity of the delay system.
  • Auditability Concerns: The lack of logging in delayFFR reduces transparency, making it difficult to trace the rationale for transaction delays.
  • Redundant Identity Checks and Logs: The 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 FFRDelayed event 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.

Potential for Transaction Censorship via Repeated Delay

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.

Low Severity

Insufficient Code Coverage

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 ISecurityValidator

The 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.

Stored Attestations Cannot Be Loaded After Previous Attestations Have Been Used

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.

Implemented Methods Are Not Part of the Interface

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:

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.

Missing Error Messages in require Statements

In 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.

Incomplete Docstrings

In AttesterControllers.sol, multiple instances of incomplete docstrings were identified:

  • The ffrIds parameter of the StoreBatch event is not documented.
  • The encryptedDetails parameter of the Detection event is not documented.
  • The configuration parameter of the registerController function is not documented.
  • The 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.

Missing Docstrings

Throughout the codebase, multiple instances of missing docstrings were identified. Some notable examples include:

Consider 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.

Missing Zero Address Checks

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:

  • The firewallAccess assignment operation within the contract FirewallPermissions contract
  • The newImplementation assignment operation within the ProxyFirewall contract
  • The roleManagerAddress assignment operation within the AttesterControllers contract
  • The inboxAddress assignment operation within the AttesterControllers contract

Consider adding a zero address check before assigning a value to a state variable.

Update: Resolved in pull requests #18 and #16.

Locked ETH in Contract

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 take msg.value as the reference value when msg.sig or ref range is zero.

Unsafe Use of DEFAULT_ADMIN_ROLE

The 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:

  • Only one account holds the DEFAULT_ADMIN_ROLE role since deployment until it is potentially renounced.
  • Enforces a two-step process to transfer the DEFAULT_ADMIN_ROLE role to another account.
  • Enforces a configurable delay between the two steps, with the ability to cancel before the transfer is accepted. The delay can be changed by scheduling.
  • It is not possible to use another role to manage the 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 AccessControlDefaultAdminRules might add needless complexity to the overall firewall management experience.

Events Emitted Regardless of Whitelist Status Change

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.

Lack of Maximum Delay for Suspicious Transactions

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 in Some Firewalls Is Inefficient and Adds Limited Value

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.

Stored Attestations Cannot Be Used By Smart Contract Accounts

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.origin is 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 same UserOperation bundle, 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.

Notes & Additional Information

Redundant Functionality Increases Contract Size and Complexity

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.

Code Clarity

Throughout the codebase, multiple opportunities for improving code quality were identified:

  • The 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.
  • The _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.

Missing Named Parameters in Mapping

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:

  • The checkpoints state variable of the Firewall contract
  • The attestations state variable of the SecurityValidator contract
  • The whitelist state variable in the Whitelist contract

Consider 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.

Non-Explicit Imports Are Used

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.

State Variable Visibility Not Explicitly Declared

Throughout the codebase, multiple instances of state variables lacking an explicitly declared visibility were identified:

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.

Unused Named Return Variables

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.

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.

Gas Optimizations

Throughout the codebase, multiple opportunities for optimizing gas usage were identified:

  • Loop Optimizations:

    • Replace the post-increment operator (i++) with the pre-increment (++i) operator to save gas where the return value of the expression is not used:
    • Accessing the length of arrays within loop conditions or bodies results in unnecessary storage or memory reads during each iteration, increasing gas consumption. There are some loops within the codebase that could benefit from caching the length of the array being iterated over:

  • Custom Errors:

    • Replace require and revert messages with custom errors to save gas:
    • The revert in AttesterControllers.sol
    • The revert in Inbox.sol

  • Hashing Optimizations:

  • Optimizable State Reads:

    • In several places, unnecessary storage reads can be optimized by caching values or changing storage references to memory:
    • 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.

Redundant Code

Throughout the codebase, multiple instances of redundant code were identified:

  • The getControllerById function is redundant because the public controllers state variable already has an automatically included getter.
  • The 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.
  • The 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.

Misleading Documentation

Throughout the codebase, multiple instances of misleading documentation were identified:

  • The 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.
  • This comment suggests that the proxy firewall implements upgradeToAndCall and the UUPSUpgradeable contract implements upgradeNextAndCall, when in fact, it is the other way around.
  • The @notice comment that contains the firewall description has an incomplete sentence.
  • This comment suggests that the _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.
  • This NatSpect comment for the delayFFR function incorrectly describes it as an event.
  • The ExternalFirewall NatSpec is outdated: the withCheckpoint modifier no longer exists, and the executeCheckpoint and setCheckpoint functions now receive additional arguments.
  • The 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.

Typographical Error

Throughout the codebase, an instance of a typographical error was identified:

  • At the end of line 33 of 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.

File and Contract Names Mismatch

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.

Floating Pragma

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.

Lack of Security Contact

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.

Interfaces Should Be Defined in Separate Files

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 @inheritdoc but 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.

Naming Issues

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.

Lack of License Specification

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.

Unnecessary Use of AccessControl in the Whitelist Contract

The 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.

Inconsistent Order Within Contracts

Throughout the codebase, multiple instances of contracts having inconsistent ordering of functions were identified:

To 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.

Lack of Indexed Event Parameters

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.

Unused Errors

Throughout the codebase, multiple instances of unused errors were identified:

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.

Unused Imports

Throughout the codebase, multiple instances of unused imports were identified:

Consider 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 Initializable

The 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.

Client Reported

Composability Risk from Divergent Trusted Attester Sets

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.

Reference Parsing Fails for Variable-Size Arguments

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:

  1. An attacker could potentially craft transactions with specially structured calldata to bypass security checks. By manipulating the position of the reference value within the variable-length data, they might be able to:
    1. Push the actual reference value out of the expected range.
    2. Insert a benign value at the expected position to pass the threshold check.
  2. The system may trigger false positives or false negatives due to misreading the reference value.
  3. This limitation restricts the types of functions that can be effectively protected by the firewall, especially those with complex parameter structures involving arrays or strings.

Consider adding support for functions with variable-length arguments in later iterations of the firewall.

Update: Acknowledged, will resolve.

Attestations Can Be Replayed

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.

 
 

Conclusion

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.

 

Read On