Biconomy - Smart Contract Wallet contest - peanuts's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L93 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L385 https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L289

Vulnerability details

Impact

Function reverts if one account or paymaster is not validated, which leads to a waste of time and gas.

Proof of Concept

EntryPoint.UserOpsPerAggregator() takes in an array of opsPerAggregator in its parameter and loops through each struct. In the function, the function _validatePrepayment() is called which validates the account and paymaster from each opsPerAggregator.

function _validatePrepayment(uint256 opIndex, UserOperation calldata userOp, UserOpInfo memory outOpInfo, address aggregator)
private returns (address actualAggregator, uint256 deadline) {


    uint256 preGas = gasleft();
    MemoryUserOp memory mUserOp = outOpInfo.mUserOp;
    _copyUserOpToMemory(userOp, mUserOp);
    outOpInfo.userOpHash = getUserOpHash(userOp);


    // validate all numeric values in userOp are well below 128 bit, so they can safely be added
    // and multiplied without causing overflow
    uint256 maxGasValues = mUserOp.preVerificationGas | mUserOp.verificationGasLimit | mUserOp.callGasLimit |
    userOp.maxFeePerGas | userOp.maxPriorityFeePerGas;
    require(maxGasValues <= type(uint120).max, "AA94 gas values overflow");


    uint256 gasUsedByValidateAccountPrepayment;
    (uint256 requiredPreFund) = _getRequiredPrefund(mUserOp);
    (gasUsedByValidateAccountPrepayment, actualAggregator, deadline) = _validateAccountPrepayment(opIndex, userOp, outOpInfo, aggregator, requiredPreFund);
    //a "marker" where account opcode validation is done and paymaster opcode validation is about to start
    // (used only by off-chain simulateValidation)
    numberMarker();

The function then continues to call _validateAccountPrepayment which does another round of checks.

function _validateAccountPrepayment(uint256 opIndex, UserOperation calldata op, UserOpInfo memory opInfo, address aggregator, uint256 requiredPrefund)
internal returns (uint256 gasUsedByValidateAccountPrepayment, address actualAggregator, uint256 deadline) {
unchecked {
    uint256 preGas = gasleft();
    MemoryUserOp memory mUserOp = opInfo.mUserOp;
    address sender = mUserOp.sender;
    _createSenderIfNeeded(opIndex, opInfo, op.initCode);
    if (aggregator == SIMULATE_FIND_AGGREGATOR) {
        numberMarker();


        if (sender.code.length == 0) {
            // it would revert anyway. but give a meaningful message
            revert FailedOp(0, address(0), "AA20 account not deployed");
        }
        if (mUserOp.paymaster != address(0) && mUserOp.paymaster.code.length == 0) {
            // it would revert anyway. but give a meaningful message
            revert FailedOp(0, address(0), "AA30 paymaster not deployed");
        }
        try IAggregatedAccount(sender).getAggregator() returns (address userOpAggregator) {
            aggregator = actualAggregator = userOpAggregator;
        } catch {
            aggregator = actualAggregator = address(0);
        }
    }

If any opsPerAggregator is not validated, the whole function will revert, leading to a waste of time and gas.

Tools Used

VSCode

Recommended Mitigation Steps

Make the loop non-atomic by using a try/catch block. If the paymaster / account is not validated, skip the loop and validate the next one. In _compensate, make sure to count the collected amount correctly if some opsPerAggregator are skipped because of validation failure.

Lines of code

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

Vulnerability details

Impact

EntryPoint address in SmartAccount.sol cannot execute certain functions because of the onlyOwner modifier.

Proof of Concept

The functions SmartAccount.execute() and SmartAccount.executeBatch() has an onlyOwner modifier.

function execute(address dest, uint value, bytes calldata func) external onlyOwner{
function executeBatch(address[] calldata dest, bytes[] calldata func) external onlyOwner{

However, both functions call another function _requireFromEntryPointOrOwner();

    _requireFromEntryPointOrOwner();

which checks if msg.sender is adderss(entryPoint()) or owner

function _requireFromEntryPointOrOwner() internal view {
    require(msg.sender == address(entryPoint()) || msg.sender == owner, "account: not Owner or EntryPoint");
}

Since execute() and executeBatch() has an onlyOwner modifier which checks if msg.sender is owner only,

modifier onlyOwner {
    require(msg.sender == owner, "Smart Account:: Sender is not authorized");
    _;
}

address(entryPoint()) cannot call the function execute() and executeBatch().

Tools Used

VSCode

Recommended Mitigation Steps

Either take out the onlyOwner modifer in the two functions or remove the _requireFromEntryPointOrOwner() check if the function intends to not allow the msg.sender to be address(entryPoint()).

Lines of code

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

Vulnerability details

Impact

Protocol cannot upgrade the upgradeable contract.

Proof of Concept

SmartAccount.sol uses ReentrancyGuardUpgradeable but not UUPSUpgradeable.

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

Tools Used

Manual Review

Recommended Mitigation Steps

import UUPSUpgradeable, inherit the contract, and initialize the contract

import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

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

    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");
+       __UUPSUpgradeable_init();
        owner = _owner;
        _entryPoint =  IEntryPoint(payable(_entryPointAddress));
        if (_handler != address(0)) internalSetFallbackHandler(_handler);
        setupModules(address(0), bytes(""));
    }

Also add storage gap for upgradeable contracts.

uint256[45] private __gap;

[L-01] Initializers can be frontrunnable

If the initializer is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call, forcing the contract to be redeployed.

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

[L-02] Contract should adhere to two-step ownership process

Setting the owner within one function can be dangerous if the address of the owner in the parameter is not set properly. Consider using a two-step process whereby the owner has to acknowledge the change before changing the ownership.

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

[L-03] Unused/Empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds

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

[L-04] OpenZeppelin's ECDSA is imported but not utilized

Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly

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

[L-05] Import exact functions instead of the whole contract itself

Example:

import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

instead of

import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

Applies to all contracts in Biconomy protocol.

SmartAccount.sol:

import "./libs/LibAddress.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import "./BaseSmartAccount.sol";
import "./common/Singleton.sol";
import "./base/ModuleManager.sol";
import "./base/FallbackManager.sol";
import "./common/SignatureDecoder.sol";
import "./common/SecuredTokenTransfer.sol";
import "./interfaces/ISignatureValidator.sol";
import "./interfaces/IERC165.sol";
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";

[N-01] Non-library/interface files should use fixed compiler versions, not floating ones

eg. 0.8.10 instead of ^0.8.10

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

[N-02] Avoid the use of sensitive terms in favour of neutral ones

Use allowlist rather than whitelist.

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

[N-03] Testing coverage is not adequate enough

 What is the overall line coverage percentage provided by your tests?:  41

Protocol should aim to achieve >95% testing coverage to make sure that all functions are in working order.