Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

`RegistryFactory.removeAttester` does not properly remove an attester if it's added multiple times

supplisec

Summary

An attester added multiple times is not removed by a single call to RegistryFactory.removeAttester.

Vulnerability Details

The function RegistryFactory.addAttester allows an owner to add an attester. However, there is not check to prevent adding an attester multiple times, even if it's already in the list attesters. This means, if an address is added X times, in order to remove it, the function RegistryFactory.removeAttester should be called X times as well.
The issue occur because RegistryFactory.addAttester does not check for duplicate entries and because RegistryFactory.removeAttester removes only the first occurrence found and there are no events or other checks that let the owner prevent such case from happening.

Relevant code:

// https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/factory/RegistryFactory.sol#L57-L69
contract RegistryFactory is Stakeable, INexusFactory {
...
function addAttester(address attester) external onlyOwner {
attesters.push(attester); //@audit: allow duplicate attesters
}
​
function removeAttester(address attester) external onlyOwner {
for (uint256 i = 0; i < attesters.length; i++) {
if (attesters[i] == attester) {
attesters[i] = attesters[attesters.length - 1];
attesters.pop();
break; //@audit: remove only the first occurrence
}
}
}

PoC

Add the same address twice in the following unit-test and the test will fail with message "Attester should be removed":

// https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/test/foundry/unit/concrete/factory/TestRegistryFactory_Deployments.t.sol#L58
​
/// @notice Tests adding and removing attesters from the registry.
function test_AddRemoveAttester() public {
address attester = address(0x456);
address attester2 = address(0x654);
vm.startPrank(FACTORY_OWNER.addr);
registryFactory.addAttester(attester);
registryFactory.addAttester(attester2);
+ registryFactory.addAttester(attester);
assertTrue(registryFactory.attesters(1) == attester, "Attester should be added");
assertTrue(registryFactory.attesters(2) == attester2, "Attester should be added");
​
registryFactory.removeAttester(attester);
assertFalse(registryFactory.attesters(1) == attester, "Attester should be removed");
vm.stopPrank();
}
​

Impact

This could lead to unexpected behaviours since the owner should remember how many times an attester is added to remove it properly. Also, if an address X is added multiple times and the owner calls RegistryFactory.removeAttester only once to remove the address X, this could lead the owner to think that the address was indeed removed while it is not.
Finally, in the case of duplicate addresses, this will increase the cost of removing the same address by calling RegistryFactory.removeAttester multiple times.

Tools Used

Manual code review

Recommendations

Consider using a map to keep track of attesters or use an approach similar to what already used for validators or executors in AccountStorage.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other
Assigned finding tags:

finding-ERC7848-add-duplicate-attester

Invalid, - Addition of attesters are admin only functionalities so if duplicate addresses are added it would consitute admin input/call validation. - ERC-7484 is in draft mode so we should not take it as the final EIP configuration yet. - Even if a mistake was made, removals can be performed by invoking `removeAttester` multiple times by the owner to completely remove a duplicate user. - In the [documentation](https://github.com/bcnmy/nexus/wiki#problems-nexus-solves), it is not noted that Nexus suite will be ERC7484 compliant.

Appeal created

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-ERC7484-sorted-duplicate-attestor-issue

Invalid, similar issue to #151 and duplicates - Addition of attesters are admin only functionalities so if duplicate addresses are added it would consitute admin input/call validation. - ERC-7484 is in draft mode so we should not take it as the final EIP configuration yet. - In the [documentation](https://github.com/bcnmy/nexus/wiki#problems-nexus-solves), it is not noted that Nexus suite will be ERC7484 compliant.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!