Event Horizon Arbitrum Franchiser Audit
July 18, 2024
Summary
Type: Governance
Timeline: From 2024-07-01 To 2024-07-05
Languages: Solidity
Total Issues: 6 (1 resolved)
Notes & Additional Information: 6 (1 resolved)
Scope
We audited the HVAX/arbfranchiser repository at commit 8caba3d.
In scope were the following files:
src
├── FranchiserFactory.sol
├── Franchiser.sol
├── FranchiserLens.sol
├── base
│ └── FranchiserImmutableState.sol
└── interfaces
├── IVotingToken.sol
├── IFranchiserLens.sol
├── IFranchiserImmutableState.sol
├── Franchiser
│ ├── IFranchiser.sol
│ ├── IFranchiserErrors.sol
│ └── IFranchiserEvents.sol
└── FranchiserFactory
├── IFranchiserFactory.sol
└── IFranchiserFactoryErrors.sol
The audit only reviewed the smart contract components intended to be deployed on-chain. Any deployment scripts, configurations, mock contracts, or tests were not reviewed.
System Overview
A recent discussion about delegating more voting power to retail investors ended in the passing of a snapshot vote which aims to delegate $ARB 7,000,000 to eventhorizoncommunity.eth. This is an EOA that the Event Horizon (EH) organization will use to represent the voters from their platform. ARB users can only delegate their entire voting power to another address. So, in order to delegate a specific amount, a contract has been created that will have the amount transferred to it and then delegate its voting power to EH.
The contract that EH provided to do this is a fork of a contract that was developed for Uniswap. It was privately audited by Trail of Bits and has a lot of functionality beyond the minimum needs of the ARB DAO. The top-level contract which the DAO will interact with is the FranchiserFactory contract. It is built to allow anyone to delegate their ERC-20 voting token, like ARB, to a delegatee. It does this by creating a Franchiser contract underneath that would mark the FranchiserFactory as the owner, hold the $ARB tokens from the caller, and call delegate on the token to transfer the voting power to the specified user (e.g., the EH EOA).
When the DAO is ready to take back the funds, a call to recall will pull the tokens out of the Franchiser and return them. There are added methods to batch delegation and recalling which we do not anticipate the DAO will need in this specific case but could use if more delegations of this sort arise. The FranchiserFactory contract will create a Franchiser contract as needed for each unique delegator/delegatee pair, while each Franchiser is set up as a minimal proxy of a Franchiser implementation contract.
Each Franchiser contract has to ability for the delegatee to subdelegate their delegated tokens to further subdelegatees. This creates a tree of Franchise contracts with the original delegator/delegatee Franchise contract at the head, each subdelegation from a delegatee at each child node, and the tokens being transferred down the tree to imbue voting power. The original delegatee can sub-delegate up to eight times with each of these sub-delegating up to four, then two, then one. Counting the maximum possible nodes in each level of the tree totals to $(1)+(8)+(8 \cdot 4)+(8 \cdot 4 \cdot 2)+(8 \cdot 4 \cdot 2) = 169$ nodes. At each Franchiser contract in this tree, the delegatee can unsubdelegate and return all of the funds transferred down the tree to their own contract and restore their full voting power delegation. The recall method mentioned above does the same thing but returns all tokens from the tree to the original delegator.
The scope also includes an optional FranchiserLens contract which contains view functions to get data from the Franchiser tree in flat formats. It is not necessary for the other contracts in scope but would give the DAO easy visibility into their delegation and any further subdeldegations.
Security Model and Trust Assumptions
This code relies on the OpenZeppelin Contracts library for cloning, checking for code size, and for set operations. It also relies on the Solmate library for ownership and transferring tokens. We assume these libraries work as described and intended and both have long on-chain history.
Notes & Additional Information
Unused Import
The import import {Franchiser} from "../../Franchiser.sol"; imports unused alias Franchiser in IFranchiserEvents.sol.
Consider removing or using any unused imports to improve the overall clarity and readability of the codebase.
Update: Acknowledged with comment “we will leave the stylistic notes as is as there are no objections to functionality.”
Unused Named Return Variable
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.
In FranchiserLens.sol, the delegation return variable for the getRootDelegation function is unused.
Consider either using or removing any unused named return variables.
Update: Acknowledged with comment “we will leave the stylistic notes as is as there are no objections to functionality.”
Lack of Events
The FranchiserFactory contract does not any emit events when new delegations are made or recalled.
Consider emitting events in functions that change the state of delegations.
Update: Acknowledged with comment “we will leave the stylistic notes as is as there are no objections to functionality.”
Inconsistent Access Control
In the Franchiser contract, subDelegate, subDelegateMany, unSubDelegate, and unSubDelegateMany will revert if called by anyone who is not the delegatee of the contract. However, subDelegateMany is the only function that is not marked as such and will revert upon an inner call to subDelegate.
To improve code clarity, consider marking subDelegateMany with the onlyDelegatee modifier like the other delegatee-restricted functions.
Update: Acknowledged with comment “we will leave the stylistic notes as is as there are no objections to functionality.”
initialize Specificity
The Franchiser contract has two functions called initialize. The first one has three parameters and is only directly called from the FranchiserFactory contract to create a top-level delegation of tokens. The second function wraps the first, has two parameters, and is only used to create subdelegations of tokens from existing Franchiser contracts.
To improve code clarity and readability, consider naming the second initialize function more specifically (e.g., initializeSubFranchise).
Update: Acknowledged with comment “we will leave the stylistic notes as is as there are no objections to functionality.”
Maximum Subdelegatees
The code has been designed for generic delegation and subdelegation of tokens. If the FranchiserFactory contract is to be used solely for this one EH delegation, it is possible to change the INITIAL_MAXIMUM_SUBDELEGATEES to one and make further subdelegations by EH impossible. This would ensure that they do not have the ability to further subdelegate to another actor who is not in the proposal but would make vote splitting by subdelegation impossible by EH (at least until fractional voting is brought to the governor).
Of course, by delegating in the first place, the DAO is already trusting EH to behave as promised. Thus, further subdelegations being used maliciously to vote in other ways appears to be a minor risk that is mitigated by the ability to revoke the funds. However, revoking is a slow process that would require a vote to pass as the L2 Governor would be the original delegator. A way around it would be to transfer the funds directly to the security council and entrust them to create the subdelegation from the FranchiserFactory contract. We are not aware, however, of a precedent or authority for the council to behave this way.
Possible ways to improve the code and tailor it specifically to Arbitrum’s needs would be to give the non-emergency security council the ability to recall the funds. This can be done by allowing the council to be specified at initial delegation and adding an ability for them to call the recall function. However, even then, a call to recall (even unSubDelegate) could be front-run to get in a last vote before losing voting power. A fix for this could be to add some sort of authorization method for the delegatee to vote only on approved votes, but this seems overly protective and operationally taxing to implement. No matter what the community chooses or how the contract iterates, we will review any on-chain proposal for construction/delegation and opine on its safety and features.
Update: Resolved in commit 101d01d.
Conclusion
This audit covered the Franchiser contracts provided by Event Horizon. They will allow the Arbitrum DAO to transfer a token through the FranchiserFactory to a Franchiser contract which will delegate it’s voting power to Event Horizon, allowing them to vote in proposals. The Franchiser contract as audited allows Event Horizon to further subdelegate their tokens. If the DAO is not interested in using this contract for further grants, we recommend the DAO remove this subdelegation ability from the contracts by setting the INITIAL_MAXIMUM_SUBDELEGATEES constant to one. We also found no security issues with the contracts and our notes contain only recommendations for coding practices. We welcome iteration on the contracts and will be auditing any changes and on-chain proposals involving them. We are grateful to the Event Horizon team for being responsive during the audit and for bringing a compelling use case to the DAO.