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

Modules may not be properly checked if user re-configures registry for the account

h2134

Summary

Modules may not be properly checked if user re-configures registry for the account, due to that the attesters in the registry are not reset.

Vulnerability Details

User can call setRegistry() through EntryPoint to configure registry for account.

function _configureRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) internal {
registry = newRegistry;
if (address(newRegistry) != address(0)) {
newRegistry.trustAttesters(threshold, attesters);
}
emit ERC7484RegistryConfigured(newRegistry);
}

The trustAttesters() on the registry will be called, as confirmed by sponsor, the implementation code of the registry contract (TrustManager) they are planning to use can be find here, and the `trustAttesters()` is as below:

function trustAttesters(uint8 threshold, address[] calldata attesters) external {
uint256 attestersLength = attesters.length;
​
...
​
TrustedAttesterRecord storage $trustedAttester = $accountToAttester[msg.sender];
​
...
​
$trustedAttester.attesterCount = uint8(attestersLength);
$trustedAttester.threshold = threshold;
$trustedAttester.attester = attesters[0];
​
attestersLength--;
​
// setup the linked list of trusted attesters
for (uint256 i; i < attestersLength; i++) {
address _attester = attesters[i];
@> $trustedAttester.linkedAttesters[_attester][msg.sender] = attesters[i + 1];
}
​
emit NewTrustedAttesters(msg.sender);
}

Assuming that user configure the registry for the first time, attesters are [attesterA, attesterB], threshold is 2, we can have $trustedAttester.linkedAttesters be like:

attesterA -> attesterB

When user re-configure the registry, they set attesters to [attesterA] and threshold to be 1, then in the code attestersLength is 1 and is later decreased to be 0, the for loop won't be enterred:

@> attestersLength--;
​
// setup the linked list of trusted attesters
for (uint256 i; i < attestersLength; i++) {
address _attester = attesters[i];
$trustedAttester.linkedAttesters[_attester][msg.sender] = attesters[i + 1];
}

The $trustedAttester.linkedAttesters remains unchanged and we have:

$trustedAttester.attesterCount: 1
$trustedAttester.threshold: 1
$trustedAttester.attester: attesterA
$trustedAttester.linkedAttesters: attesterA -> attesterB

This is problematic and can lead to that the modules are not properly checked. Let's have a look at the _check() function which is eventually called by Nexus Account:

function _check(address smartAccount, address module, ModuleType moduleType) internal view {
...
​
else if (threshold == 1) {
AttestationRecord storage $attestation = $getAttestation({ module: module, attester: attester });
if ($attestation.checkValid(moduleType)) return;
​
// if first attestation is not valid, iterate over the linked list of attesters
// and check if the attestation is valid
@> for (uint256 i; i < attesterCount; i++) {
@> attester = $trustedAttesters.linkedAttesters[attester][smartAccount];
@> $attestation = $getAttestation({ module: module, attester: attester });
@> if ($attestation.checkValid(moduleType)) return;
@> }
// if no valid attestations were found in the for loop. the module is not valid
revert InsufficientAttestations();
}
​
...
}

In our case, if the result of the attestation from attesterA is false, it will still go into the for loop, get attestation from attesterB and check the module's validity by using the attestation, this is definitely not expected despite the result.

The culprit is that the attesters for the account in the registry are not reset before re-configuring, but it's not TrustManager to blame because it's an abstract contract and its functionalities can be incomplete, also because neither trustAttesters() nor _check() is virtual hence cannot to be overridden, we can reasonably assume that its child contract in-prod provides extra functionality, e.g. function to remove linked attesters for the Nexus accounts, and this functionality needs to be called from Nexus account.

Impact

Modules may not be properly checked by the registry.

Tools Used

Manual Reivew

Recommendations

Suppose the `removeLinkedAttesters()` function is provided by the registry to remove attesters for the account, we need to call it from Nexus contract to fix the issue:

function _configureRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) internal {
+ registry.removeLinkedAttesters();
​
registry = newRegistry;
if (address(newRegistry) != address(0)) {
newRegistry.trustAttesters(threshold, attesters);
}
emit ERC7484RegistryConfigured(newRegistry);
}
Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

h2134 Submitter
over 1 year ago
h2134 Submitter
over 1 year ago
0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope
Assigned finding tags:

finding-single-validator-check-DoS

Support

FAQs

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