Smart Wallet Contracts V2 - Biconomy

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against re-entrancy attacks.

It is also suggested to use call.value method with any Reentrancy protection.

Code Location

Functions with missing Reentrancy Guard

SmartWallet:handlePayment()
SmartWallet:transfer()
SmartWalletNoAuth:handlePayment()
SmartWalletNoAuth:transfer()
EntryPoint:innerHandleOp()
StakeManager:withdrawStake()
StakeManager:withdrawTo()
SimpleWallet:addDeposit()

PARTIALLY SOLVED: The Biconomy team solved this finding by implementing nonReentrant modifiers to functions in the Code Location section. This finding will not be fixed for Account Abstraction Core contracts, as they are templates.

Commit ID: 8624fcf9a92a006a2fc6bad7bb9f834ab3641ffe

As of version 0.8.0 in Solidity, a control mechanism was implemented to prevent overflow and underflow issues for mathematical operations. Since this costs extra gas, the unchecked keyword was also implemented to remove this extra control mechanism for specific code locations.

This keyword should be used with caution to benefit from gas consumption. Otherwise, overflows/underflows for mathematical operations can occur.

Code Location

EntryPoint.sol

function _getRequiredPrefund(MemoryUserOp memory mUserOp) internal view returns (uint256 requiredPrefund) {
    unchecked { 
        //when using a Paymaster, the verificationGasLimit is used also to as a limit for the postOp call.
        // our security model might call postOp eventually twice
        uint256 mul = mUserOp.paymaster != address(0) ? 3 : 1;
        uint256 requiredGas = mUserOp.callGasLimit + mUserOp.verificationGasLimit * mul + mUserOp.preVerificationGas;

        // TODO: copy logic of gasPrice?
        requiredPrefund = requiredGas * getUserOpGasPrice(mUserOp);
    }

EntryPoint.sol

function _handlePostOp(uint256 opIndex, IPaymaster.PostOpMode mode, UserOpInfo memory opInfo, bytes memory context, uint256 actualGas) private returns (uint256 actualGasCost) {
        uint256 preGas = gasleft();
    unchecked {
        address refundAddress;
        MemoryUserOp memory mUserOp = opInfo.mUserOp;
        uint256 gasPrice = getUserOpGasPrice(mUserOp);

        address paymaster = mUserOp.paymaster;
        if (paymaster == address(0)) {
            refundAddress = mUserOp.sender;
        } else {
            refundAddress = paymaster;
            if (context.length > 0) {
                actualGasCost = actualGas * gasPrice;
                if (mode != IPaymaster.PostOpMode.postOpReverted) {
                    IPaymaster(paymaster).postOp{gas : mUserOp.verificationGasLimit}(mode, context, actualGasCost);
                } else {
                    // solhint-disable-next-line no-empty-blocks
                    try IPaymaster(paymaster).postOp{gas : mUserOp.verificationGasLimit}(mode, context, actualGasCost) {}
                    catch Error(string memory reason) {
                        revert FailedOp(opIndex, paymaster, reason);
                    }
                    catch {
                        revert FailedOp(opIndex, paymaster, "postOp revert");
                    }
                }
            }
        }

SOLVED: The Biconomy team solved this finding by removing the unchecked keywords directly from the above functions. That eliminates the risk of overflow.

Commit ID: e3080c259a580b3ddc205776a2aeca999850b0c0

A two-step procedure for changing roles in the contract is missing. If the address is incorrect, the new address will assume the functionality of the new role directly. There is no way to recover these roles back after these types of errors.

Code Location

SmartWallet.sol

function setOwner(address _newOwner) external mixedAuth { 
        require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero"); 
        address oldOwner = owner;
        owner = _newOwner;
        emit EOAChanged(address(this), oldOwner, _newOwner);
    }

SmartWalletNoAuth.sol

function setOwner(address _newOwner) external mixedAuth { 
        require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero"); 
        address oldOwner = owner;
        owner = _newOwner;
        emit EOAChanged(address(this), oldOwner, _newOwner);
    }

BasePaymaster.sol

function _transferOwnership(address newOwner) internal virtual { 
        address oldOwner = owner;
        owner = newOwner;
        emit OwnershipTransferred(oldOwner, newOwner);
    }

RISK ACCEPTED: The Biconomy team accepted the risk of this finding. It has been decided to continue with the one-step ownership change pattern.

Biconomy Smart Wallet contracts have address fields in multiple functions. These functions are missing address validations. Each address should be validated and checked to be non-zero. This is also considered a best practice.

During testing, it has been found that some of these inputs are not protected against using address(0) as the destination address.

Code Location

Missing zero address checks - Variables

StakeManager.withdrawStake(address).withdrawAddress
StakeManager.withdrawTo(address,uint256).withdrawAddress
VerifyingPaymasterFactory.deployVerifyingPaymaster(address,address,IEntryPoint).owner
VerifyingPaymasterFactory.deployVerifyingPaymaster(address,address,IEntryPoint).verifyingSigner
VerifyingPaymaster.init(IEntryPoint,address,address)._verifyingSigner
VerifyingPaymaster.init(IEntryPoint,address,address)._owner
VerifyingPaymaster.setSigner(address)._newVerifyingSigner
VerifyingPaymaster.init(IEntryPoint,address,address)._verifyingSigner
VerifyingPaymaster.init(IEntryPoint,address,address)._owner
VerifyingPaymaster.setSigner(address)._newVerifyingSigner
StakeManager.withdrawStake(address).withdrawAddress
StakeManager.withdrawTo(address,uint256).withdrawAddress
BasePaymaster.setEntryPoint(IEntryPoint)._entryPoint

SOLVED: The Biconomy team solved this finding by adding require checks to prevent the use of zero addresses.

Commit ID: 10d3933d5b9a461b85aebb9343e92ed94ac74970

The requiredTxGas function was declared to return uint256 as a return variable after a successful call. That function calls execute() function, and that function was designed to return bool, not uint256. Therefore, it does not return any variables at all. It is important to validate these return variables. In this case, calling these functions can break any integrations or composability.

Code Location

SmartWalletNoAuth.sol

function requiredTxGas(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation
    ) external returns (uint256) { 
        // We don't provide an error message here, as we use it to return the estimate
        require(execute(to, value, data, operation, gasleft()));
    }

SOLVED: The Biconomy team solved this finding by removing the return variable from the requiredTxGas function.

Commit ID: 508c44ed749f7c4b06e704d6f6b4e13db9099634

The project contains many instances of floating pragma. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, either an outdated compiler version that might introduce bugs that affect the contract system negatively or a pragma version too recent which has not been extensively tested.

During the audit, it has seen all contracts use ^0.8.0 as pragma version.

SOLVED: This finding was solved by updating the Solidity version from ^0.8.0 to 0.8.12. All caret symbols have been removed from these contracts.

Commit ID: 3b882439d3981db3b1c992fefa18e4b0f80746d4

It has been observed all for loops in the protocol were not optimized properly. Having not optimized for loops can cost too much gas usage.

These for loops can be optimized with suggestions above:

1. In Solidity (pragma 0.8.0 and later), adding unchecked keyword for arithmetical operations can reduce gas usage on contracts where underflow/underflow is unrealistic. It is possible to save gas by using this keyword on multiple code locations.
2. In all for loops, the index variable is incremented using +=. It is known that, in loops, using ++i costs less gas per iteration than +=. This also affects incremented variables within the loop code block.
3. Do not initialize index variables with 0, Solidity already initializes these uint variables as zero.

Check the Recommendation section for further details.

Code Location

Optimizable For Loops - LoC list

core/EntryPoint.sol:#L81
core/EntryPoint.sol:#L87
core/EntryPoint.sol:#L107
core/EntryPoint.sol:#L119
core/EntryPoint.sol:#L140

SOLVED: The Biconomy team solved the issue by optimizing these for loops as per the suggestion above.

Commit ID: 0dc8ee23591fd54318abdd8d892ab84958847cd0

The BasePaymaster contract uses a modifier to control functions are called by EntryPoint address. The other onlyOwner modifier uses _msgSender() function to return msg.sender address. However, the _requireFromEntryPoint modifier uses msg.sender directly. Only one of these pattern should be used to increase readability.

Code Location

BasePaymaster.sol

function _requireFromEntryPoint() internal virtual {
        require(msg.sender == address(entryPoint)); 
    }

SOLVED: The Biconomy team solved this finding by replacing the msg.sender to _msgSender() function.

Commit ID: f44e1592bbaa90569fd10f4418a4534fa7ff2acd

Some variables in Smart Wallet contracts are only set for once. The immutable keyword consumes less gas usage. It might be useful to declare these variables as immutable to optimize gas usage in the protocol.

Code Location

WalletFactory.sol

address internal _defaultImpl;

VerifyingPaymasterFactory.sol

address internal payMasterImp;

SOLVED: The Biconomy team solved this finding by adding immutable keyword for constant variables.

Commit ID: d7501353283b9a98295d58e49e051f14f4b4be1b

Revert messages longer than 32 bytes use extra PUSH32 instruction to push strings. Since this instruction use extra gas cost, it is not recommended to use revert messages longer than 32 bytes.

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas usage when the revert condition has been met.

Code Location

Long revert messages - LoC list

SmartWallet.sol:#L72
SmartWallet.sol:#L101
SmartWallet.sol:#L119
WalletFactory.sol:#L18
SmartWalletNoAuth.sol:#L72
SmartWalletNoAuth.sol:#L101
SmartWalletNoAuth.sol:#L119
MultiSend.sol:#L27
VerifyingPaymasterFactory.sol:#L14
VerifyingPaymaster.sol:#L81
BasePaymaster.sol:#L53
WhitelistModule.sol:#L20
WhitelistModule.sol:#L26
WhitelistModule.sol:#L46
WhitelistModule.sol:#L49
EntryPoint.sol:#L256
EntryPoint.sol:#L276
EntryPoint.sol:#L277
EntryPoint.sol:#L304

ACKNOWLEDGED: The Biconomy team acknowledged this finding.

There are numerous unused variables, events, and imports on the code base. These variables should be cleaned up from the code if they have no purpose. Clearing these variables can reduce gas usage during deployment of contracts. Also, clearing unneeded comments will increase readability of the code.

Code Location

Unused comments, contracts and imports - LoC list

Greeter.sol:#L0 - Unused Contract
BaseSmartWallet.sol:#L18 - Unneeded comment line
WalletFactory.sol:#L4 - Unneeded comment line
IAggregatedWallet.sol:#L4,L6 - Unused import
BasePaymaster.sol:#L88 - Unused variables

ACKNOWLEDGED: The Biconomy team acknowledged this finding.

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Code Location

Open TODOs - LoC list

SmartWallet.sol:#L140
SmartWalletNoAuth.sol:#L140
EntryPoint.sol:#L266

SOLVED: The Biconomy team solved this finding by removing open TODOs from their contracts.

Commit ID: 5c22c474c90737611cd99d280eb69464cb235d2e