Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Unbounded loop in `RegistryFactory::removeAttester` to remove the attester can lead to DOS

sheep

Description:

There is no limit to how many attesters there can be and attesters can be added through RegistryFactory::addAttester which are then stored in RegistryFactory::attesters array. An attester can be removed using the function RegistryFactory::removeAttester function which loops over the RegistryFactory::attesters array and compares the attester provided as parameter to the each attester in the array, if they match it pops the attester from array and breaks out of loop but if the attester to be removed is at the end of array and RegistryFactory::attesters array is large enough that by looping over each value, call can exceed block gas limit, which can render attesters at the end of array, unable to be removed.

Impact

Unable to remove attester at the end of RegistryFactory::attesters array. attester can be malicious

Proof of Concept:

function test_RemoveAttesterCanLeadToDOS() public {
vm.txGasPrice(1);
address attester = address(0x456);
address attester2 = address(0x654);
​
vm.startPrank(FACTORY_OWNER.addr);
registryFactory.addAttester(attester);
registryFactory.addAttester(attester2);
vm.stopPrank();
​
uint256 gasStartFirst = gasleft();
vm.startPrank(FACTORY_OWNER.addr);
registryFactory.removeAttester(attester2);
vm.stopPrank();
uint256 gasEndFirst = gasleft();
uint256 deltaFirst = (gasStartFirst - gasEndFirst) * tx.gasprice;
​
address attester3 = address(0x568);
address attester4 = address(0x758);
address attester5 = address(0x987);
address attester6 = address(0x987);
address attester7 = address(0x486);
address attester8 = address(0x567);
address attester9 = address(0x561);
address attester10 = address(0x562);
address attester11 = address(0x563);
address attester12 = address(0x564);
address attester13 = address(0x565);
address attester14 = address(0x566);
vm.startPrank(FACTORY_OWNER.addr);
registryFactory.addAttester(attester3);
registryFactory.addAttester(attester4);
registryFactory.addAttester(attester5);
registryFactory.addAttester(attester6);
registryFactory.addAttester(attester7);
registryFactory.addAttester(attester8);
registryFactory.addAttester(attester9);
registryFactory.addAttester(attester10);
registryFactory.addAttester(attester11);
registryFactory.addAttester(attester12);
registryFactory.addAttester(attester13);
registryFactory.addAttester(attester14);
vm.stopPrank();
​
uint256 gasStartSecond = gasleft();
vm.startPrank(FACTORY_OWNER.addr);
registryFactory.removeAttester(attester14);
vm.stopPrank();
uint256 gasEndSecond = gasleft();
uint256 deltaSecond = (gasStartSecond - gasEndSecond) * tx.gasprice;
​
​
assert(deltaSecond > deltaFirst);
​
​
}

Recommended Mitigation:

  1. Use a mapping to keep the track of attesters index in the array mapping(address => uint256) use this while addind attester and associate its address to index at which it is being added. when removing the attester use this mapping to check the index of address in array then pop it directly instead of looping over each entry.

  2. Use better searching algorithm

Updates

Lead Judging Commences

0xnevi Lead Judge
11 months ago
0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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