Biconomy - Smart Contract Wallet contest - gz627's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol#L5-L6 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

Impact

On verifying signatures, the outdated EIP1271_MAGIC_VALUE is used EIP1271_MAGIC_VALUE = 0x20c13b0b for verifying EIP1271 signed signatures. Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/interfaces/ISignatureValidator.sol

4:  contract ISignatureValidatorConstants {
5:      // bytes4(keccak256("isValidSignature(bytes,bytes)")
6:      bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;

    abstract contract ISignatureValidator is ISignatureValidatorConstants {
        /**
         * @dev Should return whether the signature provided is valid for the provided data
         * @param _data Arbitrary length data signed on the behalf of address(this)
         * @param _signature Signature byte array associated with _data
         *
         * MUST return the bytes4 magic value 0x20c13b0b when function passes.
         * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
         * MUST allow external calls
         */
19:      function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4);
    }
    }

As current EIP1271 requires function isValidSignature MUST return the bytes4 magic value 0x1626ba7e when function passes, using outdated magic value will result in failures of all transactions signed by contract following EIP1271.

Proof of Concept

The magic value change is due to function isValidSignature signature update. The previous signature is isValidSignature(bytes,bytes), while current one is isValidSignature(bytes32,bytes).

The EIP1271: Link to EIP1271

File link: https://eips.ethereum.org/EIPS/eip-1271

      // bytes4(keccak256("isValidSignature(bytes32,bytes)")
      bytes4 constant internal MAGICVALUE = 0x1626ba7e;

      /**
       * @dev Should return whether the signature provided is valid for the provided hash
       * @param _hash      Hash of the data to be signed
       * @param _signature Signature byte array associated with _hash
       *
       * MUST return the bytes4 magic value 0x1626ba7e when function passes.
       * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
       * MUST allow external calls
       */ 
      function isValidSignature(
        bytes32 _hash, 
        bytes memory _signature)
        public
        view 
        returns (bytes4 magicValue);
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

To make changes to relevant functions following the current EIP2171 specification.

QA Report

Non-Critical Issues

C4 checked:

[NC-1] Emit event when receiving ETH

It's a good practice to emit an event whenever a state variable changes especially when receiving ETH. Function SmartAccount.receive() can emit an event to indicate receiving ETH, e.g. emit ReceivedETH(from, to, amount). The event can be defined as:

The receive function can be:

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

    event ReceivedETH(address indexed from, address indexed to, uint256 indexed amount);

    receive() external payable {
        emit ReceivedETH(msg.sender, address(this), msg.value);
    }

---

[NC-2] Missing checks for address(0) in low-level call

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

477:    function _call(address target, uint256 value, bytes memory data) internal {
478:        (bool success, bytes memory result) = target.call{value : value}(data);
            if (!success) {
                assembly {
                    revert(add(result, 32), mload(result))
                }
            }
        }

In the above function, target is not verified. So, if there is no code at target address, the low-levell call target.call() will always return true. This has some implications:

1. Leading to permenant loss of ETH if target is mistakenly set to address(0). In this case, it's a good practice to verify: target != address(0);
2. If this function is to call a smart contract, i.e. target is supposed to ba an address of a smart contract, and if target is mistakenly set to a non-existing address (i.e. there is no code at target address), the low-level call target.call() will always return true, this breaks the function's logic. In this case, it is recommended to verify that the code size at target is larger than 0.

---

[NC-3] Redundant verification

Instances (2)

In SmartAccount contract, there are two function protected by onlyOwner. In the function body, they further restrict access by call _requireFromEntryPointOrOwner() on Line 461 and 466. Since onlyOwner has stricter restriction than _requireFromEntryPointOrOwner(). The latter can be omitted.

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

        function execute(address dest, uint value, bytes calldata func) external onlyOwner{
461:        _requireFromEntryPointOrOwner();  //@audit This line can be remove
            _call(dest, value, func);
        }

        function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{
466:        _requireFromEntryPointOrOwner();  //@audit This line can be remove
            require(dest.length == func.length, "wrong array lengths");
            for (uint i = 0; i < dest.length;) {
                _call(dest[i], 0, func[i]);
                unchecked {
                    ++i;
                }
            }
        }

L461 and L466 can be removed.

---

[NC-4] Versioning

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

36:    string public constant VERSION = "1.0.2"; // using AA 0.3.0

In Contract SmartAccount, VERSION is defined as a constant, which means the version number is fixed. It's alright if it is intended. However, there is a updateImplementation function which upgrades functionality of the contract. It also emits an event: emit ImplementationUpdated(address(this), VERSION, _implementation);. If VERSION is a constant, it does not make much sense to include a constant in an event.

It is a good practice to change the VERSION information accordingly while upgrading. It might be a good choice to offer an option to change the VERSION number in updateImplementation function so that VERSION reflects current base wallet implementation status.

---

Low Risk Issues

[L-1] Unspecific compiler version pragma

Don’t use pragma solidity ^0.8.12 when you set the compiler version yourself. This makes it ambiguous to verifiers which version of solidity was used. Only use this pattern for library code where you are not the one to compile it.

From security perspective there's always a trade-off between new code fixing known issues and potentially introducing new bugs in new releases. It is recommended to use a version that is released for a while instead of a nightly released version, i.e., to avoid from using pragma solidity ^0.x.x; or pragma solidity >=0.8.12;. In this project, it is recommended to use version 0.8.17. So the recommended compiler version pragma is: pragma solidity 0.8.17;

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/BaseAccount.sol

2: pragma solidity ^0.8.12;

All other solidity files (except for Libraries) should make the same change.

[L-2] mixedAuth modifier may have potential issues

Contract SmartAccount uses mixedAuth to protect privileged functions like setOwner, updateImplementation, updateEntryPoint. The modifier mixedAuth allows either the owner of the contract or the contract itself can access to these protected functions. However, in the current contract, there is no way for the contract itself to call these functions since there is no pre-coded code that can call these functions yet. If there is no plan to implement new code to enable the contract itself to call these privileged functions, it is a good practice to change mixedAuth to onlyOwner. This is because mixedAuth relaxes the restriction. It may unintentionally introduce risks to allow access to privileged functions. For example, if there is a need for a specific group of users to call an internal function _call() which makes low level call on provided address. The function may look like this:

    function someFunction(address target, bytes memory calldata) external onlySpecificGroup {
        ...some code...
        _call(target, 0, calldata);
    }

Note: the above function is protected by onlySpecificGroup. Function _call() is an existing function in SmartAccount contract.

The above function will open doors for onlySpecificGroup to access to all privileged functions protected by mixedAuth since target can be set to the address of SmartAccount contract, and calldata can be encoded to call privileged functions. And the msg.sender received by the privileged function is the address of the calling contract, i.e. contract SmartAccount.

Suggestions for mitigation:

1. Change mixedAuth to onlyOwner if there is no necessary to implement extra code. Don't leave potential risks in the code.
2. Take extra cautions when implementing functional functionality that uses mixedAuth.

Gas Optimizations

[GAS-1] Applying unchecked operations where no overflow/underflow possible

Instances (13)

Applying unchecked operations on the index increment in for-loops: Instances (7)

In addtion to the for-loop gas optimization in the C4audit output, a further gas optimization can be achieved to save another 63/120 gas per loop. For stack variables like uint256 index, each unchecked{++index;} saves some gas compared with ++index;. The saved gas amount varies under different situations - 120 without compiler optimization, and 63 with compiler optimization (200 runs). The index of a for-loop won't overflow so it is safe to apply unchecked increment. A typical optimized for-loop looks like:

...
uint256 length = someArray.length;
for (uint256 index; index<length;){
  ...do sth with someArray[index]...
  unchecked { ++index; }
}

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol

74:         for (uint256 i = 0; i < opslen; i++) {

80:         for (uint256 i = 0; i < opslen; i++) {

100:         for (uint256 i = 0; i < opasLen; i++) {

107:         for (uint256 a = 0; a < opasLen; a++) {

112:             for (uint256 i = 0; i < opslen; i++) {

128:         for (uint256 a = 0; a < opasLen; a++) {

134:             for (uint256 i = 0; i < opslen; i++) {

Applying unchecked operations where no overflow/underflow: Instances (6)

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol

//@audit Refer to L106 (opIndex = 0); `opIndex` increments in a limited for-loop
114:                 opIndex++;

//@audit Refer to L127
136:                 opIndex++;

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

//@audit Won't overflow: `gasleft()` is the amount of gas available for execution, `msg.data.length` is the calldata length
200:    uint256 startGas = gasleft() + 21000 + msg.data.length * 8;

//@audit Each Nonce incremented from zero, won't overflow
216:    nonces[batchId]++;

Link to code

FIle: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/base/ModuleManager.sol

//@audit L121 restricts `moduleCount < pageSize`
124:    moduleCount++;

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol

//@audit see L117: `require(withdrawAmount <= info.deposit, "Withdraw amount too large");``
118:        info.deposit = uint112(info.deposit - withdrawAmount);

---

[GAS-2] Gas optimizatin for SmartAccount.execTransaction

Function SmartAccount.execTransaction is a key one that is frequently called. So, gas optimization for this function is important. Statements of a code block in this function can be optimized to save over 7000 gas, which is significant:

Link to code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

        {
            bytes memory txHashData =
                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
212:                nonces[batchId]  //@audit This line can combine with L216 here to `nonces[batchId]++`
                );
            // Increase nonce and execute transaction.
            // Default space aka batchId is 0
216:        nonces[batchId]++;
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }

Combing L212 wieh L216 can save over 7000 gas. Optimized code:

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

        {
            bytes memory txHashData =
                encodeTransactionData(
                    // Transaction info
                    _tx,
                    // Payment info
                    refundInfo,
                    // Signature info
212:                nonces[batchId]++  //@audit Optimized by combining with L216
                );
            // Increase nonce and execute transaction.
            // Default space aka batchId is 0
            //@audit L216 is removed
            txHash = keccak256(txHashData);
            checkSignatures(txHash, txHashData, signatures);
        }

---

[GAS-3] Gas optimizatin for SmartAccount.requiredTxGas

Link to original code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

363:    function requiredTxGas(
            address to,
            uint256 value,
            bytes calldata data,
            Enum.Operation operation
        ) external returns (uint256) {
            uint256 startGas = gasleft();
            // We don't provide an error message here, as we use it to return the estimate
            require(execute(to, value, data, operation, gasleft()));
372:        uint256 requiredGas = startGas - gasleft();
            // Convert response to string and return via error message
374:        revert(string(abi.encodePacked(requiredGas)));
375:    }

Two optimizations can be applied to the above function:

1. Combime L373 and L374;
2. Apply unchecked math operation since startGas >= gasleft(). Optimized code:

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

363:    function requiredTxGas(
            address to,
            uint256 value,
            bytes calldata data,
            Enum.Operation operation
        ) external returns (uint256) {
            uint256 startGas = gasleft();
            // We don't provide an error message here, as we use it to return the estimate
            require(execute(to, value, data, operation, gasleft()));
            // Convert response to string and return via error message
            unchecked {
                revert(string(abi.encodePacked(startGas - gasleft())));
            }
375:    }

---

[GAS-4] Gas optimizatin for SmartAccount._validateSignature

Link to original code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

506:    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
        internal override virtual returns (uint256 deadline) {
            bytes32 hash = userOpHash.toEthSignedMessageHash();
            //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
            // solhint-disable-next-line avoid-tx-origin
            require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
512:        return 0;  //@audit Remove this line
        }

Remove L512 in the above function saves gas. This line is redundant since the default value of deadline is 0.

---

[GAS-5] Using named variable returns saves gas

Instances (2)

Using named variable returns reduces code size, saves deployment cost, and is less error prone (missing the return statement ).

Link to orginal code

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

139 /// @dev Returns the chain id used by this contract.
140 function getChainId() public view returns (uint256) {  //@audit Specify returning variables
        uint256 id;  //@audit This line can be removed when using named variable returns
        // solhint-disable-next-line no-inline-assembly
        assembly {
            id := chainid()
        }
        return id;  //@audit This line can be removed when using named variable returns
    }

Optimized code:

File: https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol

139 /// @dev Returns the chain id used by this contract.
140 function getChainId() public view returns (uint256 id) {  //@audit Specify returning variable: `id`
        // solhint-disable-next-line no-inline-assembly
        assembly {
            id := chainid()
        }
    }

Other instances: