number of txs of excutors must be excutores + 1 but this loop will +1 in every cycle

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/a6424230052fc47c4215200c19a8eef9b07dfccc/contracts/src/core/ConsoleOpBuilder.sol#L127-L128

Vulnerability details

Impact

number of txns of excutors must be excutores + 1 but this loop will +1 in every cycle

the code structure is designed the way the number of txns of excutors in enableExecutorPluginOnSubAccount function counts the length of executors txns and it should be excutors + 1 but it does increase in every cycle of loop

 while (i < _numberOfExecutors) {
            txns[i + 1] = Types.Executable({
                callType: Types.CallType.CALL,
                target: executorRegistry,
                value: 0,
                data: abi.encodeCall(ExecutorRegistry.registerExecutor, (subAccount, executors[i]))
            });

            unchecked {
                ++i;
            }
        }

Proof of Concept

function enableExecutorPluginOnSubAccount(address subAccount, address[] memory executors) external view returns (bytes memory) { uint256 _numberOfExecutors = executors.length; uint256 _numberOfTransactions = _numberOfExecutors + 1;

    Types.Executable[] memory txns = new Types.Executable[](_numberOfTransactions);

    bytes memory enableModuleViaModuleExec = abi.encodeCall(
        IGnosisSafe.execTransactionFromModule,
        (
            subAccount,
            0,
            abi.encodeCall(
                IGnosisSafe.enableModule, (AddressProviderService._getAuthorizedAddress(_EXECUTOR_PLUGIN_HASH))
                ),
            SafeHelper._parseOperationEnum(Types.CallType.CALL)
        )
    );

    txns[0] = Types.Executable({
        callType: Types.CallType.CALL,
        target: subAccount,
        value: 0,
        data: enableModuleViaModuleExec
    });

    address executorRegistry = AddressProviderService._getRegistry(_EXECUTOR_REGISTRY_HASH);
    uint256 i;
    while (i < _numberOfExecutors) {
        txns[i + 1] = Types.Executable({
            callType: Types.CallType.CALL,
            target: executorRegistry,
            value: 0,
            data: abi.encodeCall(ExecutorRegistry.registerExecutor, (subAccount, executors[i]))
        });

        unchecked {
            ++i;
        }
    }

    return abi.encodeCall(IGnosisMultiSend.multiSend, (SafeHelper._packMultisendTxns(txns)));
}

Tools Used

vscode

Recommended Mitigation Steps

• consider not adding + 1 in every cycle of while loop instead do it at the end

Assessed type

Loop

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

It's because of:

https://github.com/code-423n4/2023-10-brahma/blob/a6424230052fc47c4215200c19a8eef9b07dfccc/contracts/src/core/ConsoleOpBuilder.sol#L117

        txns[0] = Types.Executable({

[alex-ppg]

The relevant contract is actually out-of-scope as per the official contest scope.

Regardless, the described issue is incorrect as the txns array actually wishes to retain the first entry empty to-be-assigned afterwards as @raymondfam has correctly stated.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope