code423n4 commented 1 year ago

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.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #499

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor acknowledged