Biconomy - Smart Contract Wallet contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L57 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L34 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L38

Vulnerability details

Impact

Wallet deployment is vulnerable to cross-chain frontrun and front-run

Proof of Concept

We need to look into the wallet deployment function.

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
	bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
	}
	require(address(proxy) != address(0), "Create2 call failed");
	// EOA + Version tracking
	emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

/**
 * @notice Deploys wallet using create and points it to _defaultImpl
 * @param _owner EOA signatory of the wallet
 * @param _entryPoint AA 4337 entry point address
 * @param _handler fallback handler address
*/ 
function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){ 
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
	}
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

the deploy wallet use create opcode to create smart contract, the deployCountFactualWallet uses create2 to create smart contract.

An important difference lies in how the address of the new contract is determined. With CREATE the address is determined by the factory contract's nonce. Everytime CREATE is called in the factory, its nonce is increased by 1. With CREATE2, the address is determined by an arbitrary salt value and the init_code.

The wallet deployment is vulnerable to cross-chain front-run.

For example, a user use deployWallet to deploy a wallet in chain A,

The user assume he also controls the wallet in chain B so he sent the asset to chain B's wallet address that has not been deployed yet.

A hacker detects his transaction and frunt-run the wallet deployment in chain B.

A hacker can know the nonce of factory contract by seeing how many transaction is deployed via the factory contract.

The hacker can then copy the deploymentData and front-run the user in chain B and become owner of the wallet.

Precisely what happen in the wintermute hack back last year.

https://rekt.news/wintermute-rekt/

For the wallet that use create2, a malicious user can extract salt because he know the salt is computed below

bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));

and he can copy the deploymentData and deploy a wallet in chain B and call BaseSmartAccount(proxy).init to become the owner of the smart account.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol add chain id when generating salt and not use create opcode when deployment the proxy and when the wallet address is pre-computed in getAddressForCounterfactualWallet in Factory contract.

bytes32 salt = keccak256(abi.encodePacked(_owner, block.chainid, address(uint160(_index))));

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L57 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L38

Vulnerability details

Impact

Wallet deployment is vulnerable to deployment frontrunning

Proof of Concept

The function below is front-runnable.

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
	bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
	}
	require(address(proxy) != address(0), "Create2 call failed");
	// EOA + Version tracking
	emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

and

    /**
 * @notice Deploys wallet using create and points it to _defaultImpl
 * @param _owner EOA signatory of the wallet
 * @param _entryPoint AA 4337 entry point address
 * @param _handler fallback handler address
*/ 
function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){ 
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
	}
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

An important difference lies in how the address of the new contract is determined. With CREATE the address is determined by the factory contract's nonce. Everytime CREATE is called in the factory, its nonce is increased by 1. With CREATE2, the address is determined by an arbitrary salt value and the init_code.

When the transaction is pending in the mempool, a user can decode the transaction and get the deploymentData and the salt for the wallet that use create2 opcode, for the wallet that use create opcode, the user can use the a smart contract that have the same nonce as the factory contract, then deploy the wallet with higher gas fee.

The issue is that the deployCounterFactualWallet can revert and isAccountExist[proxy] will not be correctedly updated.

The malicious user can also call init to become the owner of the smart account.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol validate the signature to make sure the msg.sender match the owner when deploy the wallet to avoid front-running.

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Impact

The contract signature can be forged by signer

Proof of Concept

Let us look into the implementation for execTransaction in SmartAccount.sol

function execTransaction(
	Transaction memory _tx,
	uint256 batchId,
	FeeRefund memory refundInfo,
	bytes memory signatures
) public payable virtual override returns (bool success) {
	// initial gas = 21k + non_zero_bytes * 16 + zero_bytes * 4
	//            ~= 21k + calldata.length * [1/3 * 16 + 2/3 * 4]
	uint256 startGas = gasleft() + 21000 + msg.data.length * 8;
	//console.log("init %s", 21000 + msg.data.length * 8);
	bytes32 txHash;
	// Use scope here to limit variable lifetime and prevent `stack too deep` errors
	{
		bytes memory txHashData =
			encodeTransactionData(
				// Transaction info
				_tx,
				// Payment info
				refundInfo,
				// Signature info
				nonces[batchId]
			);
		// Increase nonce and execute transaction.
		// Default space aka batchId is 0
		nonces[batchId]++;
		txHash = keccak256(txHashData);
		checkSignatures(txHash, txHashData, signatures);
	}

which calls checkSignatures:

function checkSignatures(
	bytes32 dataHash,
	bytes memory data,
	bytes memory signatures
) public view virtual {
	uint8 v;
	bytes32 r;
	bytes32 s;
	uint256 i = 0;
	address _signer;
	(v, r, s) = signatureSplit(signatures, i);
	//review
	if(v == 0) {
		// If v is 0 then it is a contract signature
		// When handling contract signatures the address of the contract is encoded into r
		_signer = address(uint160(uint256(r)));

		// Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
			// This check is not completely accurate, since it is possible that more signatures than the threshold are send.
			// Here we only check that the pointer is not pointing inside the part that is being processed
			require(uint256(s) >= uint256(1) * 65, "BSA021");

			// Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
			require(uint256(s) + 32 <= signatures.length, "BSA022");

			// Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
			uint256 contractSignatureLen;
			// solhint-disable-next-line no-inline-assembly
			assembly {
				contractSignatureLen := mload(add(add(signatures, s), 0x20))
			}
			require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023");

			// Check signature
			bytes memory contractSignature;
			// solhint-disable-next-line no-inline-assembly
			assembly {
				// The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
				contractSignature := add(add(signatures, s), 0x20)
			}
			require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
	}
	else if(v > 30) {
		// If v > 30 then default va (27,28) has been adjusted for eth_sign flow
		// To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
		_signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
		require(_signer == owner, "INVALID_SIGNATURE");
	} else {
		_signer = ecrecover(dataHash, v, r, s);
		require(_signer == owner, "INVALID_SIGNATURE");
	}
}

there are three cases determined by the value of v, in the second and third case, the code validates if the signature comes from the owner.

else if(v > 30) {
	// If v > 30 then default va (27,28) has been adjusted for eth_sign flow
	// To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
	_signer = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
	require(_signer == owner, "INVALID_SIGNATURE");
} else {
	_signer = ecrecover(dataHash, v, r, s);
	require(_signer == owner, "INVALID_SIGNATURE");
}

However, if the first case, there is no such verification and whether the signature is valid wilil fully be determined by signer.

This line can be easily bypassed no matter what the input of data and contractSignature is:

require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");

As long as the return value of the isValidSignature is EIP1271_MAGIC_VALUE

For example, the contract below can bypass the line above without any verification.

contract Signer {

 bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;

  function isValidSignature(
    bytes calldata _data,
    bytes calldata _signature)
    external
    view
    returns (bytes4 magicValue) {
        return EIP1271_MAGIC_VALUE;
    }

}

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow the full contract signature without verifing that the signature is authorized by the owner of the wallet.

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L264 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L239

Vulnerability details

Impact

Gas refund amount can be inaccurate if the gas payment ERC20 token is not 18 decimals.

Proof of Concept

In the current implementation, the code handle the gas refund payment logic below:

if (refundInfo.gasPrice > 0) {
	//console.log("sent %s", startGas - gasleft());
	// extraGas = gasleft();
	payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
	emit WalletHandlePayment(txHash, payment);
}

which calls:

function handlePayment(
	uint256 gasUsed,
	uint256 baseGas,
	uint256 gasPrice,
	uint256 tokenGasPriceFactor,
	address gasToken,
	address payable refundReceiver
) private nonReentrant returns (uint256 payment) {
	// uint256 startGas = gasleft();
	// solhint-disable-next-line avoid-tx-origin
	address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
	if (gasToken == address(0)) {
		// For ETH we will only adjust the gas price to not be higher than the actual used gas price
		payment = (gasUsed + baseGas) * (gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
		(bool success,) = receiver.call{value: payment}("");
		require(success, "BSA011");
	} else {
		payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
		require(transferToken(gasToken, receiver, payment), "BSA012");
	}
	// uint256 requiredGas = startGas - gasleft();
	//console.log("hp %s", requiredGas);
}

If the gasToken is address(0), the gas refund is paid in ETH. the value gasUsed, baseGas and tx.gasprice are all in 18 decimals because the ETH is in 18 decimals.

The gas price is assumed to be calculated in 18 decimals as well

However, if the gasToken is not address(0) and the gas payment is settled in ERC20 token, the token decimals can be less than 18 decimals or more than 18 decimals.

According to

https://github.com/d-xo/weird-erc20#low-decimals

Some tokens have low decimals (e.g. USDC has 6). Even more extreme, some tokens like Gemini USD only have 2 decimals.

and

https://github.com/d-xo/weird-erc20#high-decimals

Some tokens have more than 18 decimals (e.g. YAM-V2 has 24).

However, the gasUsed and baseGas and gasPrice is still in 18 decimalls.

If the ERC20 token is not in 18 decimals, the payment amount can be over-valued if the ERC20 token is less than 18 deciamls or under-valued if the ERC20 token is more than 18 decimals.

Let us use USDC (a token that has decimals 6) as an example:

consider the code below:

payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);

let us say, the gasUsed is 21000 WEI, base Gas is 20000 WEI, gasPrice is 1000000 WEI, tokenGasPriceFactor is 10.

payment = 21000 WEI * 20000 WEI * 1000000 WEI / 10 = 4100000000 WEI

If the payment is setlled in a 18 decimals token, for example, BNB, 4100000000 WEI is a fraction of 1000000000000000000

However, if the payment is settled in USDC, 4100000000 WEI is around 4100 USDC, if we use 4100000000 / (10 ** 6).

Clearly we over-refund the gas payment.

If the Smart Account hold enough ERC20 balance, for enough 4100 UDSC, the gas refund payment can sliently go through.

the math division by tokenGasPriceFactor can reduce the impact of this issue but there is no boundary check and precision check built in-place for this parameter tokenGasPriceFactor

The code cannot the token that is more than 18 decimals as well, because the math division by tokenGasPriceFactor only scale the payment amount down but if the token is more than 18 decimals, the payment should be scaled up

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol only settle the payment in ETH to avoid such issue or whitelist and validate the payment precision beforing using the token to settle the gas refund.

ReentrancyGuardUpgradeable is not initalized in the init function of the SmartAccount

Line of Code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L28

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166

Vulnerability and recommended fix

The SmartAccount inherits from ReentrancyGuardUpgradeable

contract SmartAccount is 
     Singleton,
     BaseSmartAccount,
     IERC165,
     ModuleManager,
     SignatureDecoder,
     SecuredTokenTransfer,
     ISignatureValidatorConstants,
     FallbackManager,
     Initializable,
     ReentrancyGuardUpgradeable
    {

However, in the init function of the SmartAccount, the ReentrancyGuardUpgradeable is not initalized.

    // init
    // Initialize / Setup
    // Used to setup
    // i. owner ii. entry point address iii. handler
    function init(address _owner, address _entryPointAddress, address _handler) public override initializer { 
        require(owner == address(0), "Already initialized");
        require(address(_entryPoint) == address(0), "Already initialized");
        require(_owner != address(0),"Invalid owner");
        require(_entryPointAddress != address(0), "Invalid Entrypoint");
        require(_handler != address(0), "Invalid Entrypoint");
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

If in fact, if the SmartAccount is not upgradeable, no need to use the ReentrancyGuardUpgradeable, just use the regular reentrancy guard should be sufficient, otherwise, the recommended fix is init the ReentrancyGuardUpgradeable inside the init function

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/25aabd286e002a1526c345c8db259d57bdf0ad28/contracts/security/ReentrancyGuardUpgradeable.sol#L40

function __ReentrancyGuard_init() internal onlyInitializing {
	__ReentrancyGuard_init_unchained();
}

Gas payment token suffer from division by zero error if the tokenGasPriceFactor is set to 0

Line of Code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L264

Vulnerability and recommended fix

When handling the gas refund payment in ERC20, the transaction suffer from division by zero error if the tokenGasPriceFactor is set to 0

if (refundInfo.gasPrice > 0) {
	//console.log("sent %s", startGas - gasleft());
	// extraGas = gasleft();
	payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
	emit WalletHandlePayment(txHash, payment);
}

which calls:

function handlePayment(
	uint256 gasUsed,
	uint256 baseGas,
	uint256 gasPrice,
	uint256 tokenGasPriceFactor,
	address gasToken,
	address payable refundReceiver
) private nonReentrant returns (uint256 payment) {
	// uint256 startGas = gasleft();
	// solhint-disable-next-line avoid-tx-origin
	address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
	if (gasToken == address(0)) {
		// For ETH we will only adjust the gas price to not be higher than the actual used gas price
		payment = (gasUsed + baseGas) * (gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
		(bool success,) = receiver.call{value: payment}("");
		require(success, "BSA011");
	} else {
		payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
		require(transferToken(gasToken, receiver, payment), "BSA012");
	}
	// uint256 requiredGas = startGas - gasleft();
	//console.log("hp %s", requiredGas);
}

note the line:

payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);

the tokenGasPriceFactor (refundInfo.tokenGasPriceFactor) should not be set to 0.

We recommend the check and validate the tokenGasPriceFactor is not set to 0 before transaction executes.