Security Audit
November 2nd, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Farcaster's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from October 23, 2023 to October 27, 2023.
The purpose of this audit is to review the source code of certain Farcaster Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
Severity | Count | Acknowledged | Won't Do | Addressed |
---|---|---|---|---|
Low | 3 | 1 | - | 2 |
Code Quality | 7 | 2 | - | 5 |
Gas Optimization | 2 | - | 1 | 1 |
Farcaster was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The Farcaster protocol utilizes various privileged actors:
The following source code was reviewed during the audit:
d82a095085e92baea7f6c1c760be8c5ea36c3b57
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
./src/Bundler.sol |
|
./src/IdManager.sol |
|
./src/IdRegistry.sol |
|
./src/KeyManager.sol |
|
./src/KeyRegistry.sol |
|
./src/RecoveryProxy.sol |
|
./src/StorageRegistry.sol |
|
./src/interfaces/IBundler.sol |
|
./src/interfaces/IIdManager.sol |
|
./src/interfaces/IIdRegistry.sol |
|
./src/interfaces/IKeyManager.sol |
|
./src/interfaces/IKeyRegistry.sol |
|
./src/lib/Guardians.sol |
|
Click on an issue to jump to it, or scroll down to see them all.
KeyManager
price logic is not consistent with other modules
deadline
property for Signature is inaccurate
fid = 0
addFor
is not bound to a specific fid
extraStorage
could be implemented in the IdManager
Bundler
would need to be redeployed even if only one manager gets changed
fid
validation to the migration process
whenNotPaused
modifier can be removed from IdManager
functions
Bundler
performs redundant logic for registers without extraStorage
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
Severity | Description |
---|---|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
KeyManager
price logic is not consistent with other modules
Note: This issue was initially discovered by the Farcaster team within the scoped PR.
Reference: KeyManager.sol#L134, StorageRegistry.sol#L504
The StorageRegistry
price logic has a differentiated price use for the cases where there is a price change triggered in the same block. This allows to keep the price consistent across all transactions within that block since the price update will take effect in the following block and the prevEthUsdPrice
value will be used.
However, the KeyManager
price uses the StorageRegistry:ethUsdPrice
variable, making the price for this potential edge case different from that it would be for the other fee calculations.
deadline
property for Signature is inaccurate
Reference: Signatures.sol#L22
Signatures._verifySig
performs the following expiration check:
if (block.timestamp >= deadline) revert SignatureExpired();
At the same time, the comment for error InvalidSignature()
says:
/// @dev Revert when the block.timestamp is ahead of the signature deadline.
error SignatureExpired();
The comment doesn’t align with the actual logic, as the above check also reverts when block.timestamp
exactly matches the deadline
.
Remediations to Consider
Consider changing the above check to block.timestamp > deadline
instead of >=
.
fid = 0
Reference: StorageRegistry.sol#L431, StorageRegistry.sol#L456, StorageRegistry.sol#L645, StorageRegistry.sol#L656
In StorageRegistry.sol
, the functions rent()
, batchRent()
, credit()
, and batchCredit()
allow renting storage to fid = 0
. However, as fid = 0
is an invalid fid
, it leads to sending ether to the contract without renting storage to any fid
.
Remediations to Consider
Consider adding checks for the above functions to not allow fid = 0
.
We don’t intend to update the StorageRegistry as part of these changes, but will validate this in the next version. For now there is limited impact beyond callers unintentionally sending ether to the protocol.
Reference: docs.md?plain=1#L192
The documentation states:
The KeyRegistry is deployed in the trusted state where keys may not be registered by anyone except the owner.
However, this is not the case since adding keys via KeyManager.add
and KeyManager.addFor
is not restricted and can be called by anybody even in the trusted state.
Consider updating documentation to properly reflect behavior.
Note that
IdRegistry
andKeyRegistry
now are actually deployed in a paused state. Updated the docs to reflect the latest and remove references to trusted caller.
Reference: docs.md
The documentation well explains the different roles of the system and also mentions under which assumptions they are acting. Since the recent changes, the protocol supports an additional role called “Guardians”, who have the privilege to pause contracts and can only be added by the contract owner.
Consider adapting the documentation to also mention the Guardian role (this is especially relevant for the “Assumption” section)
Reference: IdRegistry.sol#L6, KeyRegistry.sol#L5
The following import is not required and can be removed from IdRegistry.sol
and KeyRegistry.sol
:
import {Pausable} from "openzeppelin/contracts/security/Pausable.sol";
addFor
is not bound to a specific fid
Reference: KeyManager.sol#L239
The TYPEHASH
used by the KeyManager.addFor
function is defined as follows:
bytes32 public constant ADD_TYPEHASH = keccak256(
"Add(address owner,uint32 keyType,bytes key,uint8 metadataType,bytes metadata,uint256 nonce,uint256 deadline)"
);
Since there is no fid
encoded in the TYPEHASH, the signature is not bound to a certain fid. This opens up the possibility for the following edge case:
fid_123
sig1
, allowing someone else to add a key on her behalf.fid_456
. Until this point, sig1
has not been executed.sig1
is not bound to a certain fid, the signature can still be used via KeyManager.addFor
to add a key to fid_456
.The above scenario allows for adding a key to fid_456
. However, Alice created a signature with the intention to authorize adding a key to fid_123
.
Consider adding the fid to the TYPEHASH
to prevent the above scenario.
This is an intentional design tradeoff that makes it possible to register an FID and add a key in a single transaction, without first knowing a user’s FID. We accept that this has the consequence described in the finding, and users should interpret these actions as “add key to current FID.”
We added a partial mitigation by exposing a public
useNonce
function on all contracts that use nonce-protected EIP712 signatures. Users who want to protect against this scenario can bump their nonce to invalidate previous signatures.
extraStorage
could be implemented in the IdManager
Reference: Bundler.sol#L97, IdManager.sol#L109
The current function flow for a Bundler:register()
call, goes through the following steps:
register
in the IdManager
to register a new fid
and rent
one unit of storage, required for registration.add
in the KeyManager
to add the signers passed in the call.rent
in the StorageRegistry
to rent any additional storage needed, passed in the extraStorage
input parameter.This could be improved by passing additional storage into the IdManager
and adding it to the required initial unit of storage since the IdManager
already interacts with the StorageRegistry
contract to rent storage, simplifying the interactions of the Bundler
.
Bundler
would need to be redeployed even if only one manager gets changed
Reference: Bundler.sol#L47 , Bundler.sol#L52
The v3.1.0 update aims to create a more flexible system that enables the registry owner
to safely update the manager
modules. However, due to the immutability of the Bundler
's idManager
and keyManager
variables, any changes made to the managers in the registry would require the bundler to be redeployed.
Consider having setter functions inside the Bundler
contract to allow updating these modules easily.
We’re OK redeploying Bundler contracts if necessary, since it’s cheap to do so on Optimism, and prefer to minimize our own control over the contract dependencies.
fid
validation to the migration process
Reference: Bundler.sol#L144
It is paramount, that users being migrated have the same fid
on the new contracts. The migration on-chain logic doesn’t ensure this and solely relies on passing the UserData[]
array in correct order to the Bundler.trustedBatchRegister
function so that fid
matches with the migrated contract.
Consider adding the corresponding fid
s to UserData
struct so that during migration the correct fid
can be validated against the current contract in Bundler.trustedBatchRegister
.
We don’t want to do this onchain, because it introduces an additional dependency on the old contract, but we will include validation against the previous contract state in our offchain migration scripts.
whenNotPaused
modifier can be removed from IdManager
functions
Reference: IdManager.sol#L104, IdManager.sol#L120, IdManager.sol#L134
In IdManager.sol
, the functions register()
, registerFor()
, and trustedRegister()
utilize the whenNotPaused
modifier, but also internally call idRegistry.register
which by itself uses its corresponding internal whenNotPaused
modifier. This leads to unnecessary gas consumption as the same modifier logic is evaluated twice.
Consider removing the whenNotPaused
modifier from above functions.
Pausing the IdRegistry pauses registration, transfers, and recovery, but pausing the IdGateway only pauses registrations. In the event of an change to the IdGateway, we want the ability to pause the old gateway contract without having to pause the IdRegistry.
Bundler
performs redundant logic for registers without extraStorage
Reference: Bundler.sol#L97, Bundler.sol#L104, StorageRegistry.sol#L504
For users that register their fid
through the Bundler
but do not want additional storage than the required for registration, the logic will still perform price checks and additional calls to the StorageRegistry
.
Consider adding a differentiated logic when extraStorage == 0
to avoid unnecessary logic execution, which users will pay.
Implemented as part of Q-5.
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the Farcaster team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.