https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/factory/BiconomyMetaFactory.sol#L70-L81
Summary
This vulnerability arises from mishandling msg.value sent by the user during the deployment process done by the BiconomyMetaFactory. It leads to the loss of all funds sent by the users to their contract accounts after deployment. Moreover, these funds cannot be retrieved by the factory since there is no mechanism to send the locked ETH out of the contract. Therefore, the ETH value will be locked permanently.
Vulnerability Details
In the contract factory BiconomyMetaFactory, the payable function deployWithFactory takes a factory address and factoryData as inputs and makes a call to that factory if it has been whitelisted before. This call invokes the function createAccount on the factory.
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);
require(success, CallToDeployWithFactoryFailed());
assembly {
createdAccount := mload(add(returnData, 0x20))
}
}
As shown in this function, it is a payable function allowing any user who calls it to send ETH with the call. Then, it calls the function createAccount on any factory, which uses the function LibClone.createDeterministicERC1967. This function takes msg.value as the first parameter to send this value to the deployed account, as shown here:
function createAccount(bytes calldata initData, bytes32 salt) external payable override returns (address payable) {
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)
}
-> (bool alreadyDeployed, address account) = LibClone.createDeterministicERC1967(msg.value, ACCOUNT_IMPLEMENTATION, actualSalt);
}
-> function createDeterministicERC1967(uint256 value, address implementation, bytes32 salt)
internal
returns (bool alreadyDeployed, address instance)
{
Due to the fact that the BiconomyMetaFactory does not send the msg.value with the call to the factory, this will lead to locking all the ETH that has been sent to the BiconomyMetaFactory, which is considered a huge loss of funds for the users who use the BiconomyMetaFactory for account deployment.
Proof of Concept (PoC)
Please include this test in the file TestAccountFactory_Deployment.t.sol, and run the test:
function test_DeployAccount_CreateAccount_1() public {
BootstrapConfig[] memory validators = BootstrapLib.createArrayConfig(address(VALIDATOR_MODULE), initData);
BootstrapConfig memory hook = BootstrapLib.createSingleConfig(address(0), "");
bytes memory saDeploymentIndex = "0";
bytes32 salt = keccak256(saDeploymentIndex);
bytes memory _initData = BOOTSTRAPPER.getInitNexusScopedCalldata(validators, hook, REGISTRY, ATTESTERS, THRESHOLD);
address payable expectedAddress = FACTORY.computeAccountAddress(_initData, salt);
vm.expectEmit(true, true, true, true);
emit AccountCreated(expectedAddress, _initData, salt);
bytes memory factoryData = abi.encodeWithSelector(FACTORY.createAccount.selector, _initData, salt);
vm.prank(user.addr);
console.log("the balance of the meta factory before the deployment = ", address(META_FACTORY).balance);
-> address payable deployedAccountAddress = META_FACTORY.deployWithFactory{ value: 1 ether }(address(FACTORY), factoryData);
console.log("the balance of the meta factory before the deployment = ", address(META_FACTORY).balance);
console.log("the balance of the account after deployment = ", deployedAccountAddress.balance);
assertEq(address(META_FACTORY).balance, 1 ether);
vm.expectRevert();
assertEq(deployedAccountAddress.balance, 1 ether);
}
Consider reading the logs:
Ran 1 test for test/foundry/unit/concrete/factory/TestAccountFactory_Deployments.t.sol:TestAccountFactory_Deployments
[PASS] test_DeployAccount_CreateAccount_1() (gas: 281438)
Logs:
the balance of the meta factory before the deployment = 0
the balance of the meta factory before the deployment = 1000000000000000000
the balance of the account after deployment = 0
Impact
This vulnerability will cause a huge loss of funds for all users who try to deploy their accounts using the BiconomyMetaFactory and fund their accounts after deployment .
Tools Used
VSCode, Foundry
Recommendations
To mitigate this vulnerability, the BiconomyMetaFactory should send the msg.value to the factory during the low-level call. Consider modifying the deployWithFactory function as follows:
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))
}
}
