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

Low issues

bauchibred

L-01 RegistryFactory#computeAccountAddress() would not work on zksync

Summary

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/factory/RegistryFactory.sol#L136-L147

function computeAccountAddress(bytes calldata, bytes32) external view override returns (address payable expectedAddress) {
bytes32 actualSalt;
assembly {
let ptr := mload(0x40)
let calldataLength := sub(calldatasize(), 0x04)
mstore(0x40, add(ptr, calldataLength))
calldatacopy(ptr, 0x04, calldataLength)
actualSalt := keccak256(ptr, calldataLength)
}
expectedAddress = payable(LibClone.predictDeterministicAddressERC1967(ACCOUNT_IMPLEMENTATION, actualSalt, address(this)));
}
​

This function is used to compute the expected address if a Nexus contract using the factory's deterministic deployment algorithm, now whereas this would work in most chains protocol would deploy to, it wouldn't work on zksync, because on zkSync ERA address deviation/creation is not done in the exact same way as is done on Ethereum, see https://docs.zksync.io/build/developer-reference/ethereum-differences/evm-instructions#address-derivation.

Impact

RegistryFactory#computeAccountAddress() would not work as expected on zksync.

Would be key to note that the computeAccountAddress() has 3 different implementations in scope, all of which would be susceptible to this bug case.

Tools Used

Manual review

Recommendations

Reimplement the way the computeAccountAddress() is being done if the chain where this is queried is zksync.

L-02 Executors could be diluted to 0 and cause a DOS in Nexus.sol

Vulnerability Details

See https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/base/ModuleManager.sol#L207-L245

function _installValidator(address validator, bytes calldata data) internal virtual withRegistry(validator, MODULE_TYPE_VALIDATOR) {
if (!IValidator(validator).isModuleType(MODULE_TYPE_VALIDATOR)) revert MismatchModuleTypeId(MODULE_TYPE_VALIDATOR);
_getAccountStorage().validators.push(validator);
IValidator(validator).onInstall(data);
}
​
function _uninstallValidator(address validator, bytes calldata data) internal virtual {
SentinelListLib.SentinelList storage validators = _getAccountStorage().validators;
​
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
​
// Check if the account has at least one validator installed before proceeding
// Having at least one validator is a requirement for the account to function properly
require(!(prev == address(0x01) && validators.getNext(validator) == address(0x01)), CannotRemoveLastValidator());
​
validators.pop(prev, validator);
IValidator(validator).onUninstall(disableModuleData);
}
​
function _installExecutor(address executor, bytes calldata data) internal virtual withRegistry(executor, MODULE_TYPE_EXECUTOR) {
if (!IExecutor(executor).isModuleType(MODULE_TYPE_EXECUTOR)) revert MismatchModuleTypeId(MODULE_TYPE_EXECUTOR);
_getAccountStorage().executors.push(executor);
IExecutor(executor).onInstall(data);
}
​
function _uninstallExecutor(address executor, bytes calldata data) internal virtual {
(address prev, bytes memory disableModuleData) = abi.decode(data, (address, bytes));
_getAccountStorage().executors.pop(prev, executor);
IExecutor(executor).onUninstall(disableModuleData);
}

These are all functionalities used to install and uninstall executors & validators, issue however is that wehreas there is acheck to ensure that the protocol is not left in a state where there are no validators, this check is not present when they are removing executors, which could cause for a DOS when all the executors are uninstalled in the functionality below from Nexus.sol: https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/Nexus.sol#L137-L152

function executeFromExecutor(
ExecutionMode mode,
bytes calldata executionCalldata
) external payable onlyExecutorModule withHook withRegistry(msg.sender, MODULE_TYPE_EXECUTOR) returns (bytes[] memory returnData) {
(CallType callType, ExecType execType) = mode.decodeBasic();
// check if calltype is batch or single or delegate call
if (callType == CALLTYPE_SINGLE) {
returnData = _handleSingleExecutionAndReturnData(executionCalldata, execType);
} else if (callType == CALLTYPE_BATCH) {
returnData = _handleBatchExecutionAndReturnData(executionCalldata, execType);
} else if (callType == CALLTYPE_DELEGATECALL) {
returnData = _handleDelegateCallExecutionAndReturnData(executionCalldata, execType);
} else {
revert UnsupportedCallType(callType);
}
}

Impact

This could cause for the protocol to be put in an unwanted state where there are no executors, and functionalities that need executors, from Nexus.sol would be unavailable.

Recommended Mitigation Steps

Consider introducing a similar check for executor removals just as is present for validator removals.

Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-zksync-create-create2-opcode

Valid medium, since there is non-functionality on zkSync (cannot create accounts) since it is stated as follows > Blockchains: > - Ethereum/Any EVM

Support

FAQs

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