Biconomy: Nexus

Lines of code

https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/factory/RegistryFactory.sol#L61-L69

Impact

When removing an attester from the attester array by calling removeAttester(), it is not ensured that the attesters are “in order” after the call. This will result in a revert of the function “createAccount()” when the registry function “trustAttesters” is called and prevent new accounts from being created by the factory.

Proof of Concept

The RegistryFactory is used to create new Smart Accounts. Users can add specific modules to such a Smart Account to e.g. automate specific task. For a module to be addable to a Smart Account, it needs to be seen as “trusted” in a registry. For a module to be “trusted” it needs to have enough attestations from specific attesters in the registry. The factory determines which attesters are considered and how many attestations are needed.

The registry used to check if a module is trusted will follow ERC-7484 for maximal compatibility.

When the factory creates a new Smart Account, it first checks if the modules are trusted in the registry, adds all modules to the account. As a final step, the registry for the account is set and the array of attesters as well as the number of needed attestations is saved in the registry by calling registry.trustAttesters(threshold, attesters); .

According to ERC-7484, when calling trustAttesters, the provided array of attesters MUST be unique and sorted and the Registry MUST revert if they are not.

The issue arises from the fact that when the owner of the RegistryFactory calls removeAttester(), the removed attester is replaced by the last attester in the array and the last attester in the array is removed. This, in most cases, will breaking the sorted nature of the attesters array.