Security Audit
August 18th, 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 August 3, 2023 to August 9, 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 |
---|---|---|---|---|
Medium | 2 | - | - | 2 |
Low | 5 | 2 | - | 3 |
Code Quality | 10 | 3 | - | 7 |
Farcaster was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
2cf6706011915352f57bc2ed884d9efd28ccecf0
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
./src/Bundler.sol |
|
./src/FnameResolver.sol |
|
./src/IdRegistry.sol |
|
./src/KeyRegistry.sol |
|
./src/StorageRegistry.sol |
|
./src/lib/Signatures.sol |
|
./src/lib/TransferHelper.sol |
|
./src/lib/TrustedCaller.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
rentedUnits
accounting in continuousCredit()
function
_TRANSFER_TYPEHASH
prevents all fid transfers
KeyRegistry
does not restrict public interactions in the trusted phase
Bundler
contract
StorageRegistry
the owner can set an arbitrary fixedEthUsdPrice
KeyRegistry
contract can be deployed without any constraints on thegracePeriod
fixedEthUsdPrice
the users could potentially be charged a stale rent price
Bundler
contract could be independent and reusable
Bundler:register()
does not check for zero storageUnits
input
StorageRegistry
contract should be able to pause and unpause external functions
StorageRegistry:withdraw()
doesn’t revert for zero amounts
KeyRegistry
contract could be converted to a function modifier
StorageRegistry
contract
fid
recovery
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. |
rentedUnits
accounting in continuousCredit()
function
In the StorageRegistry
contract, the contract operator may credit a sequence of fids with free storage units. Natspec documentation and implementation indicate that start and end arguments are inclusive range boundaries, meaning that fids equal to start and end should be granted free credits too.
The current implementation does indeed credit all fids in the mentioned range with free credits. However, due to an off-by-one error in calculating the length of the range, the calculated value of totalUnits
is smaller by units
amount than what it is supposed to be. As a result, the invariant that rentedUnits
is always equal to the sum of all units rented ever will be invalidated. In addition, the check related to the maxUnits
may be ineffective.
Remediations to Consider
Update len
calculation to len = end - start + 1
and update for loop condition to i < len
.
_TRANSFER_TYPEHASH
prevents all fid transfers
In the IdRegistry
contract, _TRANSFER_TYPEHASH
is set to an incorrect value. The underlying event signature is incorrect for the first argument - instead of address from
, it should be uint256 fid
.
bytes32 internal constant _TRANSFER_TYPEHASH =
keccak256("Transfer(address from,address to,uint256 nonce,uint256 deadline)");
As a result, _TRANSFER_TYPEHASH
within the contract has an incorrect value, and consequently, the request digest, which _TRANSFER_TYPEHASH
is a composite part of, will have an incorrect value. Due to that, _verifyTransferSig()
execution will fail unless callers replicate the same error in generating _TRANSFER_TYPEHASH
on their end.
Remediations to Consider
Update _TRANSFER_TYPEHASH
with the expected signature for the arguments in use.
bytes32 internal constant _TRANSFER_TYPEHASH =
keccak256("Transfer(uint256 fid,address to,uint256 nonce,uint256 deadline)");
KeyRegistry
does not restrict public interactions in the trusted phase
As per the specification:
The KeyRegistry is deployed in the trusted state where keys may not be registered by anyone except the owner.
However, KeyRegistry
public functions add()
, addFor()
, remove()
, and removeFor()
are not being restricted by any access control.
Remediations to Consider
Consider using the whenNotTrusted()
modifier for these functions to disallow users from registering and removing keys before the open registrable phase.
This is by design. Registrations are intentionally restricted during the trusted phase, but if a user owns an FID, we want them to be able to add and remove keys even during the trusted phase. The meaningful authorization check here should be ensuring that the caller owns an FID.
The trusted caller’s special privilege in the context of this contract is the ability to add a key on behalf of an FID without owning that FID or providing a signature from the owner. Once the trusted phase ends, we revoke that privilege and the only way to add/remove a key is to own the associated FID or provide a signature from the owner.
The different meanings of “trustedOnly” between this contract and the Bundler/IdRegistry are a bit confusing. We’ll add an explanation to documentation.
Bundler
contract
The Bundler
contract implements a receive()
function to allow overpayments to be sent and later refunded in the register()
logic. However, assets sent incorrectly to the contract will get stuck in this contract without recovery.
Remediations to Consider
receive()
function to check whether the msg.sender
is the StorageRent
contract and revert to avoid users mistakenly sending funds to the contract, ormsg.sender
in the storageRegistry.rent()
call so that the storageRegistry
can refund msg.sender
directly.StorageRegistry
the owner can set an arbitrary fixedEthUsdPrice
When calculating the price to rent storage units, the oracle price feeds are used to get the ETH price in USD. The price feed is acceptable if it is bound within the priceFeedMinAnswer
and priceFeedMaxAnswer
.
However, the owner of the contract can set a non-zero value for the fixedEthUsdPrice
to override the price feeds. But the fixedEthUsdPrice
is not checked to be within the acceptable min/max bounds.
Remediations to Consider
Consider checking whether the fixedEthUsdPrice
is within the acceptable range ofpriceFeedMinAnswer
and priceFeedMaxAnswer
.
KeyRegistry
contract can be deployed without any constraints on thegracePeriod
In the KeyRegistry
contract, admins can make corrections to the migrated data during the gracePeriod
.
In the documentation, the grace period is mentioned to be 24 hours
.
However, the owner can set an arbitrarily large value for the gracePeriod
during the deployment of the contract.
If gracePeriod
is arbitrarily large, the owner will be permitted to make changes well after the migration has been assumed to be completed and the hubs have switched over to this contract as the source of truth.
Remediations to Consider
Consider setting a limit of 24 hours
on the gracePeriod
in the constructor of the contract.
fixedEthUsdPrice
the users could potentially be charged a stale rent price
In the StorageRegistry
contract, if the eth/usd price feed is unavailable, it’s not possible to rent storage units. For the system to operate in such emergencies, the contract owners could set a non-zero value to the fixedEthUsdPrice
, which then acts as a substitute for the price feed.
When the price feed becomes operational again, depending upon the order of steps undertaken to switch back to the price feed, it’s possible that the users would be charged a stale rent price.
Consider the case, fixedEthUsdPrice
is set to 0, and there was no recent calls to the refreshPrice()
.
IfrefreshPrice()
were to be called in the block wherefixedEthUsdPrice
was reset and users attempt to rent storage units. The users would be charged stale prices, which were valid before the feed was deactivated.
The primary reason for this is the following code in _ethUsdPrice()
function:
/**
* We want price changes to take effect in the first block after the price
* refresh, rather than immediately, to keep the price from changing intra
* block. If we update the price in this block, use the previous price
* until the next block. Otherwise, use the latest price.
*/
return (lastPriceFeedUpdateBlock == block.number) ? prevEthUsdPrice : ethUsdPrice;
Remediations to Consider
refreshPrice()
call is performed at least one block before the fixedEthUsdPrice
is reset, which would resolve the issue of the users being charged stale prices, ORSince Chainlink price feeds have historically been pretty stable, we don’t want to add extra complexity to the contract to handle this unlikely edge case onchain, especially since it requires coordinating calls across multiple blocks. However, if we do take emergency action that disables and re-enables price feeds, we’ll take care to follow the suggestion here and call refreshPrice manually at least a block before calling setFixedEthUsdPrice.
Bundler
contract could be independent and reusable
The Bundler
contract serves as a wrapper contract to allow users and the trusted caller to register fid
, keys
for a specific fid
, and rent storage in one transaction. Currently, this contract inherits from the TrustedCaller
contract and has immutable
variables for each of the contracts it interacts with.
Since the Bundler
contract’s logic is simple and each of the counterparts performs proper access control and state sanity checks, this contract could be reusable for potential migrations and even more stateless.
Consider:
IdRegistry
, StorageRegistry
, and KeyRegistry
), allowing to update them in case of migration and reusing a deployed Bundler
.TrustedCaller
since each IdRegistry
call has proper access control checks on msg.sender
and would avoid potential different trustedOnly
states for different contracts.This would save logic execution, such as setting the trustedOnly
state differently from their counterparts and avoid subsequent redeploys of the contract with the same logic and use case.
We’d like to keep the Bundler as simple as possible, and we’d rather pay L2 gas to redeploy if necessary than add complexity to the contract.
Bundler:register()
does not check for zero storageUnits
input
If a user attempts to register
by passing zero storageUnits
, the function call will revert while performing the external StorageRegistry.rent()
call. Consider validating that the storageUnits
input parameter is higher than 0
before making the external function call.
StorageRegistry
contract should be able to pause and unpause external functions
During the process of upgrading the StorageRegistry
contract, the new contract is deployed and meant to be paused so that storage cannot be rented. And is later unpaused once the various migration steps are complete.
However, there’s no straightforward way to pause and unpause the storage contract’s functions.
Consider inheriting the Pausable
contract in StorageRegistry
to implement pause and unpause capabilities on the external functions.
We can find the TrustedCaller
states in each of the Bundler
, IdRegistry
, and KeyRegistry
contracts. Since these are independent contracts, they could have different inconsistent states for trustedOnly
and trustedCaller
.
Additionally, for the Bundler
to interact with both IdRegistry
and KeyRegistry
, its address should be set as the trustedCaller
address for both contracts.
Consider using one source of truth for the access control restrictions, potentially in the IdRegistry
, since both Bundler
and KeyRegistry
perform external calls to the IdRegistry
.
Since Warpcast is the only entity interacting with trusted functions, in practice we’ll switch the IdRegistry first.
StorageRegistry:withdraw()
doesn’t revert for zero amounts
Consider adding logic to revert the transaction if the treasurer attempts to withdraw a zero
amount.
We accept the risk here. This function is only callable by a trusted role and we accept that we might waste gas if we try to withdraw a zero amount.
Bundler:
keyRegistry
variable. Replace Address of the StorageRegistry contract
with Address of the KeyRegistry contract
keyRegistry
param in the constructor natspecregistration
parameter of register()
function has incorrect natspec with reference to from
, which doesn't exist and should be replaced with recovery
.trustedRegister()
is missing multiple params (scheme, key, metadata)IdRegistry:
Error in natspec for Register event
// Transfer signature below should be **Transfer(alice, bob, ...)**
Two Register(alice, ..., ...) cannot emit unless a **Transfer(..., alice, bob)** emits in between, where bob != alice.
Missing return param natspec for multiple functions (register
, registerFor
, verifyFidSignature
, etc.)
KeyRegistry:
Incorrect nat-spec comments for the removeFor()
function.
Replace Add a key on behalf of another fid owner
with Remove a key on behalf of another fid owner
.
Remove potentially obsolete part of the natspec comment for the Migrated
event -
Emit an event when the admin calls migrateKeys. **Used to migrate Hubs from using
off-chain signers to on-chain signers.**
Natspec comments for the Add
, Remove
, and AdminReset
events contain a typo in the keccak
wording. The extra "c" in kecca~~c~~k
should be removed.
StorageRegistry:
rent()
functionSetDeprecationTimestamp
event needs to be deleted to avoid any confusion.KeyRegistry
contract could be converted to a function modifier
This following check is performed in several functions related to adding and removing keys.
uint256 fid = idRegistry.idOf(fidOwner);
if (fid == 0) revert Unauthorized();
Consider moving the repetitive logic to a function modifier for better readability.
StorageRegistry
contract
Consider extracting price-related functionality into a separate updatable external contract. In case the external price feed contract needs to be replaced, the StorageRegistry
will not be affected, and costly migration would be prevented.
TrustedCaller
contractKeyRegistry
contractfid
recovery
In the IdRegistry
contract, the recover()
function is responsible for performing recovery on behalf of the custody address by transferring the fid
to a new address. When this happens, a Transfer
event is emitted.
However, for off-chain monitoring tools, it is not easy to differentiate a recovery versus a normal transfer event.
Consider introducing a new event to differentiate between a fid recovery and a normal transfer.
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.