c4-submissions commented 10 months ago

Lines of code

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

Vulnerability details

Impact

the nonce value is not increasing everytime

The nonce value is used to create the TypeHashHelper.Transaction struct that's passed to the _buildTransactionStructHash function. The actual value of executorNonce[execRequest.account][execRequest.executor] is incremented and stored in the _transactionStructHash, but it doesn't affect the value of executorNonce outside of this function.

So, executorNonce[execRequest.account][execRequest.executor] will not be updated in the external state by this code. It will be increased only within the context of creating the Transaction struct and computing the _transactionStructHash. The original mapping value will remain unchanged in the external state.

AND IT WILL BE SAME FOR EVER

Proof of Concept


function _validateExecutionRequest(ExecutionRequest calldata execRequest) internal {
        // Fetch executor registry
        ExecutorRegistry executorRegistry =
            ExecutorRegistry(AddressProviderService._getRegistry(_EXECUTOR_REGISTRY_HASH));

        // Check if executor is valid for given account
        if (!executorRegistry.isExecutor(execRequest.account, execRequest.executor)) {
            revert InvalidExecutor();
        }

        // Empty Signature check for EOA executor
        if (execRequest.executor.code.length == 0 && execRequest.executorSignature.length == 0) {
            // Executor is an EOA and no executor signature is provided
            revert InvalidSignature();
        }

        // Build transaction struct hash
        bytes32 _transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: execRequest.exec.target,
                value: execRequest.exec.value,
                data: execRequest.exec.data,
                operation: uint8(SafeHelper._parseOperationEnum(execRequest.exec.callType)),
                account: execRequest.account,
                executor: execRequest.executor,
                nonce: executorNonce[execRequest.account][execRequest.executor]++
            })
        );

        // Build EIP712 digest for transaction struct hash
        bytes32 _transactionDigest = _hashTypedData(_transactionStructHash);

        // Validate executor signature
        if (
            !SignatureCheckerLib.isValidSignatureNow(
                execRequest.executor, _transactionDigest, execRequest.executorSignature
            )
        ) {
            revert InvalidExecutor();
        }

        // Validate policy signature
        TransactionValidator(AddressProviderService._getAuthorizedAddress(_TRANSACTION_VALIDATOR_HASH))
            .validatePreExecutorTransaction(
            msg.sender, execRequest.account, _transactionStructHash, execRequest.validatorSignature
        );
    }

Tools Used

vscode

Recommended Mitigation Steps

consider increasing the nonce outside of the bytes32 _transactionStructHash = cause it doesn't increase it as external it just put ++ increased value of post value same value everytime

Assessed type

Other

c4-pre-sort commented 10 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #129

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The Warden appears to not have a proper understanding of how post-fix / pre-fix increment / decrement operators are parsed in Solidity. Specifically:

mapping(uint256 => mapping(address => uint256) public foo;
// The following statement 
++foo[5][msg.sender];
// Actually parses to the following
foo[5][msg.sender] = foo[5][msg.sender] + 1;
// Regardless of the context the post-fix / pre-fix operator is identified in

As such, the nonce will actually increment as expected and properly update the executorNonce mapping.