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

Centralization Risk for Trusted Owners

juggernaut63

File Location: https://github.com/Cyfrin/2024-07-biconomy/blob/d2adadc0f3105eb789329eb3c958472638692a2d/contracts/factory/RegistryFactory.sol#L57-L73

Summary

The 'RegistryFactory' contract relies on a single externally owned account (EOA) as the sole owner to perform critical functions, thereby creating a risk of centralization. This single failure can cause the contract to be compromised if the owner's private key is stolen or otherwise inaccessible.

Vulnerability Details

The original implementation of the 'RegistryFactory' contract includes important functions such as 'addAttester', 'removeAttester', and 'setThreshold', all protected by the 'onlyOwner' modifier. This design requires a single EOA to perform these sensitive operations, creating significant centralization risks. If the owner's private key is lost or compromised, the entire contract can become vulnerable.

Impact

  • Single Point of Failure

  • Increased Risk of Exploitation

  • Operational Risks

Tools Used

  • Inspection manual

  • Solidity

  • Foundry

Recommendations

To overcome this, you can change the contract implementation from using 'onlyOwner' to using a multi-signature wallet or a role-based authorization model.

Code snippet:

L57-L73

function addAttester(address attester) external onlyOwner {
attesters.push(attester);
}
​
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;
}
}
}
​
function setThreshold(uint8 newThreshold) external onlyOwner {
threshold = newThreshold;
}

Fixed code:

contract RegistryFactory {
address public safeAddress;
address[] public attesters;
uint8 public threshold;
​
modifier onlySafe() {
require(msg.sender == safeAddress, "Not authorized");
_;
}
​
constructor(address _safeAddress) {
safeAddress = _safeAddress;
}
​
function addAttester(address attester) external onlySafe {
attesters.push(attester);
}
​
function removeAttester(address attester) external onlySafe {
for (uint256 i = 0; i < attesters.length; i++) {
if (attesters[i] == attester) {
attesters[i] = attesters[attesters.length - 1];
attesters.pop();
break;
}
}
}
​
function setThreshold(uint8 newThreshold) external onlySafe {
threshold = newThreshold;
}
}

Explanation:

  1. ‘safeAddress’: The address of Gnosis Safe who will act as owner.

  2. Modifier ‘onlySafe’: Ensures that only Gnosis Safe can call certain functions.

  3. Constructor: Initializes the Gnosis Safe address when the contract is created.

Code when testing using Foundry:

contract RegistryFactory {
address public safeAddress;
address[] public attesters;
uint8 public threshold;
​
modifier onlySafe() {
require(msg.sender == safeAddress, "Not authorized");
_;
}
​
constructor(address _safeAddress) {
safeAddress = _safeAddress;
}
​
function addAttester(address attester) external onlySafe {
attesters.push(attester);
}
​
function removeAttester(address attester) external onlySafe {
for (uint256 i = 0; i < attesters.length; i++) {
if (attesters[i] == attester) {
attesters[i] = attesters[attesters.length - 1];
attesters.pop();
break;
}
}
}
​
function setThreshold(uint8 newThreshold) external onlySafe {
threshold = newThreshold;
}
​
function getAttestersLength() external view returns (uint256) {
return attesters.length;
}
}

Foundry output:

Ran 6 tests for test/RegistryFactory.t.sol:RegistryFactoryTest
[PASS] testAddAttester() (gas: 60685)
[PASS] testAddAttesterNotAuthorized() (gas: 12764)
[PASS] testRemoveAttester() (gas: 46728)
[PASS] testRemoveAttesterNotAuthorized() (gas: 12861)
[PASS] testSetThreshold() (gas: 36050)
[PASS] testSetThresholdNotAuthorized() (gas: 10717)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 56.58ms (71.56ms CPU time)

Ran 1 test suite in 121.86ms (56.58ms CPU time): 6 tests passed, 0 failed, 0 skipped (6 total tests)

Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

finding-centralization-risk

Invalid [known issue [Medium-1]](https://github.com/Cyfrin/2024-07-biconomy/issues/1)

Appeal created

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

finding-centralization-risk

Invalid [known issue [Medium-1]](https://github.com/Cyfrin/2024-07-biconomy/issues/1)

Support

FAQs

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