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

User may lose funds when creating Nexus account or executing user operations

h2134

Summary

User may lose funds when creating Nexus account or executing user operations, due to that `msg.value` is ignored.

Vulnerability Details

When user calls deployWithFactory() on BiconomyMetaFactory to create Nexus account, the factory contract specified by user will be called but msg.value is ignored:

function deployWithFactory(address factory, bytes calldata factoryData) external payable returns (address payable createdAccount) {
require(factoryWhitelist[address(factory)], FactoryNotWhitelisted());
@> (bool success, bytes memory returnData) = factory.call(factoryData);
​
// Check if the call was successful
require(success, CallToDeployWithFactoryFailed());
​
// Decode the returned address
assembly {
createdAccount := mload(add(returnData, 0x20))
}
}

When executeUserOp() is called by EntryPoint, protocol extracts inner call data from user operation and calls to the target contract with the decoded data, but msg.value is ignored:

function executeUserOp(PackedUserOperation calldata userOp, bytes32) external payable virtual onlyEntryPoint {
// Extract inner call data from user operation, skipping the first 4 bytes.
bytes calldata innerCall = userOp.callData[4:];
bytes memory innerCallRet = "";
​
// Check and execute the inner call if data exists.
if (innerCall.length > 0) {
// Decode target address and call data from inner call.
(address target, bytes memory data) = abi.decode(innerCall, (address, bytes));
bool success;
// Perform the call to the target contract with the decoded data.
@> (success, innerCallRet) = target.call(data);
// Ensure the call was successful.
require(success, InnerCallFailed());
}
​
// Emit the Executed event with the user operation and inner call return data.
emit Executed(userOp, innerCallRet);
}

When fallback() is revoked, is calltype is CALLTYPE_SINGLE, a fallback handler will be called, but again msg.value is ignore:

if (calltype == CALLTYPE_SINGLE) {
assembly {
calldatacopy(0, 0, calldatasize())
​
// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
mstore(calldatasize(), shl(96, caller()))
​
@> if iszero(call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)) {
returndatacopy(0, 0, returndatasize())
revert(0, returndatasize())
}
returndatacopy(0, 0, returndatasize())
return(0, returndatasize())
}
}

Impact

  1. User will lose funds if they call with ETH;

  2. Transaction may revert due to msg.value is not forwarded.

Tools Used

Manual Review

Recommendations

Handle msg.value properly.

  1. BiconomyMetaFactory.sol

function deployWithFactory(address factory, bytes calldata factoryData) external payable returns (address payable createdAccount) {
require(factoryWhitelist[address(factory)], FactoryNotWhitelisted());
- (bool success, bytes memory returnData) = factory.call(factoryData);
+ (bool success, bytes memory returnData) = factory.call{value: msg.value}(factoryData);
​
// Check if the call was successful
require(success, CallToDeployWithFactoryFailed());
​
// Decode the returned address
assembly {
createdAccount := mload(add(returnData, 0x20))
}
}

  1. Nexus.sol

function executeUserOp(PackedUserOperation calldata userOp, bytes32) external payable virtual onlyEntryPoint {
// Extract inner call data from user operation, skipping the first 4 bytes.
bytes calldata innerCall = userOp.callData[4:];
bytes memory innerCallRet = "";
​
// Check and execute the inner call if data exists.
if (innerCall.length > 0) {
// Decode target address and call data from inner call.
(address target, bytes memory data) = abi.decode(innerCall, (address, bytes));
bool success;
// Perform the call to the target contract with the decoded data.
- (success, innerCallRet) = target.call(data);
+ (success, innerCallRet) = target.call{value: msg.value}(data);
// Ensure the call was successful.
require(success, InnerCallFailed());
}
​
// Emit the Executed event with the user operation and inner call return data.
emit Executed(userOp, innerCallRet);
}

  1. ModuleManager.sol

if (calltype == CALLTYPE_SINGLE) {
assembly {
calldatacopy(0, 0, calldatasize())
​
// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
mstore(calldatasize(), shl(96, caller()))
​
- if iszero(call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)) {
+ if iszero(call(gas(), handler, callvalue(), 0, add(calldatasize(), 20), 0, 0)) {
returndatacopy(0, 0, returndatasize())
revert(0, returndatasize())
}
returndatacopy(0, 0, returndatasize())
return(0, returndatasize())
}
}
Updates

Lead Judging Commences

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

finding-cannot-msg.value-not-forwarded

Support

FAQs

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