Particle CS

Access Control

6.1 Removing a target from the whitelist can cause transactions in the PENDING state to become stuck

A pending transaction in EngineBlox can leave the PENDING status only through the approval paths or through the cancellation paths. Both ways verifies that the target for the transaction is whitelisted:

function _cancelTransaction(SecureOperationState storage self, uint256 txId) private {
    // @audit-issue This call blocks cancellation when the target is no longer whitelisted.
    _validateTargetWhitelist(
        self,
        self.txRecords[txId].params.executionSelector,
        self.txRecords[txId].params.target
    );

    self.txRecords[txId].status = TxStatus.CANCELLED;

    removePendingTx(self, txId);

    logTxEvent(self, txId, self.txRecords[txId].params.executionSelector);
}

This can cause a scenario where a transaction is stuck in PENDING state:

1. A requester calls txRequest(...) with target T, while T is whitelisted for executionSelector. The record is stored with status = PENDING;
2. The whitelist administrator removes T from self.functionTargetWhitelist[executionSelector];
3. Any attempt to call txDelayedApproval(...) or _txApprovalWithMetaTx(...) reverts inside _completeTransaction(...) on the whitelist check, after consuming gas for the external call;
4. Any attempt to call txCancellation(...) or txCancellationWithMetaTx(...) reverts inside _cancelTransaction(...) on the same whitelist check, before any state transition can occur;

The record is stuck in TxStatus.PENDING indefinitely. The only way to unblock it is for an administrator to re-add T to the whitelist.

Recommendation

Consider removing the call to _validateTargetWhitelist(...) from _cancelTransaction(...). Cancellation does not interact with the target and does not depend on the target being currently authorized.

Resolution

Removed _validateTargetWhitelist(...) from _cancelTransaction(...). Fixed in commit 72d67d.

6.2 Target whitelist is checked after the external call has executed

Target whitelisting is enforced through _validateTargetWhitelist(...), which checks that target is registered for executionSelector in self.functionTargetWhitelist. The goal is to restrict every approved transaction to a destination that was authorized before.

When txDelayedApproval(...) is called, the functions perform the external call first and after that they call _completeTransaction(...), which is where the whitelist is checked:

function txDelayedApproval(SecureOperationState storage self, uint256 txId, bytes4 handlerSelector)
    public
    returns (TxRecord memory)
{
    // ...
    self.txRecords[txId].status = TxStatus.EXECUTING;

    // INTERACT: External call after state update
    (bool success, bytes memory result) = executeTransaction(self, self.txRecords[txId]);

    // @audit-issue Whitelist is enforced only after the external call has run.
    _completeTransaction(self, txId, success, result);
    // ...
}

function _completeTransaction(
    SecureOperationState storage self,
    uint256 txId,
    bool success,
    bytes memory result
) private {
    // @audit-issue This check belongs before `executeTransaction(...)`, not after it.
    _validateTargetWhitelist(
        self,
        self.txRecords[txId].params.executionSelector,
        self.txRecords[txId].params.target
    );
    // ...
}

The same pattern repeats in _txApprovalWithMetaTx(...), which also calls executeTransaction(...) before _completeTransaction(...).

Two consequences follow from this ordering.

First, gas is wasted on the unhappy path. If the target is no longer whitelisted at approval time, for example because an admin removed it between request and approval, the engine still flips status to EXECUTING, forwards the configured gas to the target, and lets the target execute arbitrary code before _completeTransaction(...) reverts the entire transaction on the late whitelist check. The caller pays for all of that work even though the call should never have been issued.

Second, the late check breaks checks-effects-interactions pattern.

Recommendation

Consider moving _validateTargetWhitelist(...) so that the whitelist is enforced before any external call is made.

Resolution

The finding was valid as a gas-efficiency and CEI-ordering issue. _validateTargetWhitelist(...) was moved out of _completeTransaction(...) and is enforced before any external call on every approval path (txDelayedApproval(...) and _txApprovalWithMetaTx(...). Fixed in commit 72d67d.

6.3 In-memory writes to record.status and record.result in executeTransaction(...) have no effect

The executeTransaction(...) helper performs the low-level call associated with an approved transaction. The helper accepts the transaction record as TxRecord memory record, which is a memory copy of self.txRecords[txId] taken at the call site.

After performing the external call, the helper transitions the record status and store the returned data depending on the call outcome:

function executeTransaction(SecureOperationState storage self, TxRecord memory record)
    private
    returns (bool, bytes memory)
{
    // ...
    if (success) {
        // @audit-issue Writing to a memory copy has no effect on the persisted record.
        record.status = TxStatus.COMPLETED;
        record.result = result;

        if (record.payment.recipient != address(0)) {
            executeAttachedPayment(self, record);
        }
    } else {
        // @audit-issue Writing to a memory copy has no effect on the persisted record.
        record.status = TxStatus.FAILED;
        record.result = result;
    }

    return (success, result);
}

Because record is a memory parameter rather than a storage reference, both branches mutate a local copy that is discarded once the function returns. The persisted status and result are written only later by _completeTransaction(...), which the callers invoke after executeTransaction(...) returns. The intermediate memory writes are dead code. They are not consumed by the caller, the nested executeAttachedPayment(...) re-reads status from storage via _validateTxStatus(self, record.txId, TxStatus.EXECUTING) instead of from record.status, and any external reader of self.txRecords[txId] observes the values set by _completeTransaction(...).

Recommendation

Consider removing the assignments to record.status and record.result from executeTransaction(...), since they have no effect.

Resolution

Removed dead record.status and record.result assignments. Fixed in commit 72d67d.

6.4 validateDeadline(...) and validateMetaTxDeadline(...) behave differently when block.timestamp == deadline

The protocol uses two different helpers to decide whether the deadline field of a meta-transaction is still valid. They are intended to express the same predicate but they treat the case block.timestamp == deadline differently.

function validateDeadline(uint256 deadline) internal view {
    // @audit-issue Strict comparison rejects `block.timestamp == deadline`.
    if (deadline <= block.timestamp) revert DeadlineInPast(deadline, block.timestamp);
}

function validateMetaTxDeadline(uint256 deadline) internal view {
    // @audit-issue Non-strict comparison accepts `block.timestamp == deadline`.
    if (block.timestamp > deadline) revert MetaTxExpired(deadline, block.timestamp);
}

validateDeadline(...) is used in generateMetaTransaction(...), which is the helper that builds the MetaTransaction payload that an off-chain signer is expected to sign. validateMetaTxDeadline(...) is invoked from the meta-transaction verification path that runs inside verifySignature(...) and is reached from txApprovalWithMetaTx(...), txCancellationWithMetaTx(...), and the request-and-approve meta-tx variants.

This produces an inconsistency at the exact second equal to deadline. A meta-transaction whose deadline matches the current block timestamp at verification time is still executed normally, because validateMetaTxDeadline(...) admits the boundary. The same deadline, however, cannot be used to construct a fresh MetaTransaction payload through generateMetaTransaction(...), because validateDeadline(...) rejects it.

Recommendation

Consider aligning the two helpers on the same boundary semantics.

Resolution

Removed validateDeadline(...) and unified on validateMetaTxDeadline(...). Fixed in commit 72d67d.

6.5 validateTransactionExists(...) does not bound txId against txCounter

The validateTransactionExists(...) helper in SharedValidation is named so that callers can rely on it to confirm that a given txId corresponds to a real transaction record. Its current implementation only rejects the zero value:

function validateTransactionExists(uint256 txId) internal pure {
    if (txId == 0) revert ResourceNotFound(bytes32(uint256(txId)));
}

In EngineBlox, transaction identifiers are minted sequentially. Each new record receives txId = self.txCounter + 1 inside createTxRecord(...), and self.txCounter is then incremented in txRequest(...), so any allocated identifier lies in the inclusive range [1, self.txCounter]. Identifiers strictly greater than self.txCounter are not valid attached to any existing record, however the function accepts them as if they did.

Recommendation

Consider modifying validateTransactionExists(...) so that it also rejects any txId greater than the current self.txCounter.

Resolution

Added txCounter parameter and reject txId > txCounter. Fixed in commit 72d67d.