We performed an incremental audit on the forta-network/forta-firewall-contracts repository between commits c42acc8 and 09feff1.
In scope were the following files:
src/
├── AttesterWallet.sol
├── CheckpointExecutor.sol
├── ExternalFirewall.sol
├── Firewall.sol
├── FirewallPermissions.sol
├── FirewallRouter.sol
├── SecurityValidator.sol
├── TrustedAttesters.sol
└── interfaces
├── FirewallDependencies.sol
├── IAttesterWallet.sol
├── ICheckpointHook.sol
├── IExternalFirewall.sol
├── IFirewall.sol
├── ISecurityValidator.sol
└── ITrustedAttesters.sol
This diff-audit focused on the primary changes introduced by the Forta team:
AttesterWallet Contract: An ERC-20 upgradeable contract with extra functionality. It maintains native currency balances, called FORTAGAS, per user transaction origin and spends them when an attester stores an attestation on behalf of such an origin.FirewallRouter Contract: Introduced to enhance modularity and upgradeability for firewall management. It enables the firewall to be upgraded, allowing integrator contracts to point to the FirewallRouter instead of a specific firewall instance.TrustedAttesters Contract: A centralized registry for managing trusted attesters. It improves security by employing role-based access control mechanisms, ensuring only authorized attesters can interact with the system.SecurityValidator Contract: The storeAttestationForOrigin function has been added, allowing the storage of attestations tied to specific origins. This feature simplifies attestation handling and adds flexibility to the system.AttestationForwarder Contract: This contract has been removed.TransientSlot library instead of StorageSlot for managing transient storage.Two new contracts introduce or extend privileged roles:
DEFAULT_ADMIN_ROLE: Can grant and revoke roles such as Attester Managers and upgrade the SecurityValidator address on the AttesterWallet contract.ATTESTER_MANAGER_ROLE: Can grant and revoke the TRUSTED_ATTESTER role within TrustedAttesters.TRUSTED_ATTESTER_ROLE: Can store attestations for a beneficiary and get reimbursed for an arbitrary chargeAmount from an arbitrary chargeAccount.TrustedAttesters contract effectively holds the trusted attester role, as the contract is upgradable. The owner must retain the trusted attester role to avoid reversion of the store attestation call to the security validator contract.quantize Function Can Cause a DoSThe quantize function contains a vulnerability caused by an intermediate overflow during sequential operations in the return statement:
return ((n >> offset) << offset) + (2 ** offset) - 1;
When offset = 256 (calculated as 8 * Math.log256(n) for sufficiently large n), the operation 2 ** offset results in an overflow since 2^256 exceeds the maximum value of a uint256. This overflow affects the intermediate result of the addition:
((n >> offset) << offset) + (2 ** offset)
Because Solidity performs this addition first, the overflow in the intermediate result causes the function to revert when it encounters an overflow. The subtraction - 1 is never reached, as the overflow prevents further execution.
Consider refactoring the formula so that the full second term (2 ** offset - 1) is calculated first, in order to prevent the overflow. Additionally, consider adding fuzz tests to your test suite in order to detect edge cases in this quantize calculation.
Update: Resolved in pull request #47.
The _secureExecution function in the Firewall contract has a vulnerability that arises when the byteRange extracted from calldata represents a signed integer. In this case, the data is cast to a uint256, resulting in a loss of the original signedness.
If the byteRange represents a negative integer, its two's complement representation is treated as a large positive unsigned value when converted to uint256. This can lead to unexpected behavior during threshold checks in _checkpointActivated, such as ref >= checkpoint.threshold. Negative values, once interpreted as large unsigned integers, could either incorrectly satisfy or fail the comparison, bypassing the intended validation logic.
Consider properly documenting the behavior of calldata extraction and casting, including its implications on signedness, to ensure the firewall can reliably protect functions handling various data types.
Update: Resolved in pull request #48.
Throughout the codebase, multiple instances of missing docstrings were identified:
AttesterWallet.sol, the initialize functionFirewallRouter.sol, the firewall state variableFirewallRouter.sol, the updateFirewall functionConsider 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 in pull request #49.
When assigning an address from a user-provided parameter, it is crucial to ensure the address is not set to zero. Setting an address to zero is problematic because it has special burn/renounce semantics. This action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.
Throughout the codebase, there are multiple instances where assignment operations are missing a zero address check:
beneficiary and _securityValidator assignment operations within the contract AttesterWallet.origin assignment operation within the contract SecurityValidator.Consider adding a zero address check before assigning a state variable.
Update: Resolved in pull request #50.
Throughout the codebase, multiple instances of incomplete docstrings were identified:
AttesterWallet.sol, the withdraw function parameters amount and beneficiary are not documented.AttesterWallet.sol, the withdrawAll function parameter beneficiary is not documented.AttesterWallet.sol, the storeAttestationForOrigin function parameters chargeAccount and chargeAmount are not documented.CheckpointExecutor.sol, the attestedCall function does not have all return values documented.ExternalFirewall.sol, both executeCheckpoint functions ([1], [2]) parameter caller are not documented.Firewall.sol, the functions getAttesterControllerId, getCheckpoint and attestedCall do not have all return values documented.FirewallRouter.sol, both executeCheckpoint functions ([1], [2] parameter caller are not documented.SecurityValidator.sol, the functions getCurrentAttester, hashAttestation,executeCheckpoint and executionHashFrom do not have all return values documented.TrustedAttesters.sol, the isTrustedAttester function does not have all return values 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 in pull request #51.
Within AttesterWallet, all functions should ensure that amounts are not zero.
Consider implementing input validations in functions where parameters must be confined within specific boundaries. Furthermore, ensure that variables used across different functions are checked against the same boundaries to maintain consistency and integrity.
Update: Resolved in pull request #52.
The FirewallRouter contract inherits from FirewallPermissions, which provides an internal _updateFirewallAccess function to update the firewallAccess variable. While the firewallAccess is updated in the constructor, there is no explicit function in FirewallRouter to invoke this internal function, making it impossible to update the firewallAccess variable after contract deployment.
Consider implementing a restricted function to allow updates to firewallAccess after deployment in order to improve flexibility.
Update: Resolved in pull request #53.
The following instances were found where no license or a wrong one was specified:
TrustedAttesters.sol contractITrustedAttesters.sol interfaceSecurityValidator.sol contractThe use of // SPDX-License-Identifier: UNLICENSED in the code indicates that no explicit license has been specified. This has significant implications:
To ensure control over how your code is used, consider specifying a license and enforcing consistency on the license specified across the rest of the contracts. A license provides legal protection by establishing clear usage terms, which helps prevent misuse and encourages collaboration.
Update: Resolved in pull request #54.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.
Throughout the codebase, instances of revert and/or require messages were found. For instance:
AttesterWallet.sol file, the require statement with the message "sender is not a trusted attester". This error is also inconsistent with the same one raised on the SecurityValidator contract. Consider emitting the same custom error instead.Firewall.sol file, the require statement with the message "refStart is larger than refEnd".FirewallPermissions.sol file, the require statement with the message "caller is not firewall admin".FirewallPermissions.sol file, the require statement with the message "caller is not checkpoint manager".FirewallPermissions.sol file, the require statement with the message "caller is not logic upgrader".FirewallPermissions.sol file, the require statement with the message "caller is not checkpoint executor".FirewallPermissions.sol file, the require statement with the message "new firewall access contract cannot be zero address".For conciseness and gas savings, consider replacing require and revert messages with custom errors.
Update: Resolved in pull request #55.
Within SecurityValidator.sol, the trustedAttesters state variable lacks an explicitly declared visibility.
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 #56.
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.
Within Firewall.sol, multiple instances of unused named return variables were identified:
validator return variable of the getFirewallConfig functioncheckpointHook return variable of the getFirewallConfig functionattesterControllerId return variable of the getFirewallConfig functionConsider either using or removing any unused named return variables.
Update: Resolved in pull request #57.
Throughout the audit process, a few issues were identified, leading to recommendations aimed at improving code consistency, readability, and gas efficiency.
In addition to these recommendations, the codebase would benefit from a strengthened test suite, including fuzz tests to identify critical vulnerabilities, as well as an expansion of unit tests to achieve a test coverage rate above 95%. This is essential for adhering to the highest standards of security and reliability, thereby significantly enhancing code safety.
The Forta team's approach to the audit process was highly commendable. Their prompt responses, willingness to engage in discussions about the findings, and dedication to improving their product's security were particularly noteworthy. Despite the issues identified, the potential of the Forta Firewall remains strong, with its capability to significantly enhance DeFi security.