Nitewatch

The removeSigners function removes the signers first and then sets the new threshold.

_removeSigners(signersToRemove.toAddressBytesArray()); _setThreshold(newThreshold);

However, the internal MultiSignerERC7913._removeSigners function always performs the _validateReachableThreshold check. Since the threshold has not yet been updated at this stage, the validation is executed against the previous threshold value.

As a result, any attempt to remove enough signers that the old threshold becomes unattainable fails even if the caller passes a smaller newThreshold

Set the new threshold first and then remove the signers.

Yellow Team: Acknowledged.

A signer about to be removed can observe the pending removeSigners transaction in the mempool and front-run it by submitting a finalizeWithdraw call with higher gas. Because the removal has not yet been executed, the signer remains in the active set and passes the onlySigner modifier.

When a pending withdrawal is one approval short of the threshold, this front-run pushes the approval count over, triggering _executeWithdrawal and transferring the funds. The removeSigners transaction confirms afterward — the signer is removed, but the funds are already gone.

The remaining signers have no way to prevent this. Even if they no longer want the withdrawal to proceed — whether due to changed circumstances, a revised signer configuration, or simply reconsidering the request — rejectWithdraw is only callable after OPERATION_EXPIRY (1 hour). There is no on-chain cancellation mechanism during the active window.

Consider acknowledging this behavior so that signers are aware of it.

Yellow Team: Acknowledged.

In ThresholdCustody.sol, the _countValidApprovals function iterates all current signers and checks withdrawalApprovals[withdrawalId][s]. When a signer is removed via removeSigners, their entry in withdrawalApprovals mapping is NOT cleared, it remains true.

If that same address is later re-added via addSigners within the OPERATION_EXPIRY window of a pending withdrawal, the old approval is counted again without the re-added signer explicitly calling finalizeWithdraw.

Consider storing and validating the timestamp when the signer was added and validating it against the request creation date in _countValidApprovals, or acknowledging this behavior if it is intended.

Yellow Team: Acknowledged.

InvalidThreshold error is defined in ThresholdCustody contract but never used.

Remove unused error.

Yellow Team: Acknowledged.

In service.go, the processWithdrawal function treats a successful finalizeWithdraw transaction as a completed withdrawal. However, in the ThresholdCustody logic, a successful finalizeWithdraw call does not necessarily mean the withdrawal was executed. The function records the signer’s approval, but executes the withdrawal only when validApprovals >= request.requiredThreshold. If the threshold is not yet reached, the transaction succeeds, records an additional approval and the withdrawal remains pending. Despite this, the worker records the withdrawal in the database as approved and finalized.

This creates a mismatch between the off-chain accounting state and the actual contract state. In particular, receipt.Status == 1 only confirms that the transaction did not revert - it does not guarantee that funds were transferred or that the withdrawal was finalized. Because the database is later used to calculate hourly and daily withdrawal totals, pending withdrawals may be counted as completed withdrawals even though no assets have actually left custody. As a result, the rate-limiting mechanism can be manipulated or poisoned, causing legitimate withdrawals to be blocked based on phantom usage.

Update the worker so that it records a withdrawal only after the contract has actually finalized it on-chain. processWithdrawal should stop using receipt.Status == 1 as the completion condition and instead verify that a matching WithdrawFinalized event (with success == true) was emitted. If finalizeWithdraw succeeds but no such event is observed, the request should remain pending and must not be included in the withdrawal totals used for rate limiting.

Yellow Team: Acknowledged.

The startWithdraw function creates a withdrawal request but does not count the calling signer as an approver. The signer who initiates the withdrawal must call finalizeWithdraw separately to register their own approval.

The caller already passed the onlySigner check, so the contract knows they are a valid signer, but their approval is not recorded. For a threshold of 1 this means two transactions are needed where one should be enough. For higher thresholds it adds an extra round-trip for the initiator.

Record the caller's approval inside startWithdraw so the initiating signer does not need a second transaction.

Yellow Team: Acknowledged.

In processWithdrawal, the worker attempts to call rejectWithdraw immediately when a withdrawal fails the off-chain policy checks. However, the contract allows rejection only after the request has expired:

require(block.timestamp > request.createdAt + OPERATION_EXPIRY, WithdrawalNotExpired());

As a result, when the worker receives a fresh WithdrawStarted event and immediately tries to reject it, the call will typically revert because the expiry window has not yet passed. This means the worker cannot actually reject policy-violating withdrawals at the moment they are detected. Once the rejection attempt fails, the service records the event as an error and treats it as processed, which prevents the same request from being retried later when it does become eligible for rejection. Consequently, this leaves policy-violating withdrawals unresolved in a pending state and may allow them to be finalized by other signers before expiry.

If immediate off-chain policy veto is required, add a dedicated on-chain path. Introduce a separate function that is callable only by a dedicated Nitewatch role operated by the off-chain worker, not by arbitrary signers. If ThresholdCustody is intended to remain committee-driven, do not use the current worker with ThresholdCustody.

Yellow Team: Acknowledged.

newThreshold is defined as uint256 in all hardcoded type hashes (e.g., SET_THRESHOLD_TYPEHASH), while the corresponding function parameters use uint64 newThreshold. Although this does not affect the resulting hash, aligning the types would reduce the risk of confusion or incorrect signing by off-chain tooling.

Ensure the newThreshold type is consistent between the function parameters and the hardcoded type hashes.

Yellow Team: Acknowledged.

The withdrawal rate-limiting logic exists only in the off-chain worker and is not enforced by ThresholdCustody itself. This is especially problematic when the threshold is set to 1, because startWithdraw immediately executes the withdrawal in that case. As a result, funds can leave custody before the worker has any opportunity to apply the configured policy checks, allowing the off-chain rate limits to be bypassed.

If withdrawal rate limiting is intended security control, it should be enforced on-chain before a withdrawal can be executed. The contract should validate the limits during the withdrawal flow, rather than relying exclusively on the off-chain worker. If it is intended to keep policy evaluation off-chain, the contract should be adjusted so that execution cannot occur without an explicit approval from the off-chain component. As a short-term mitigation, avoid configurations with threshold = 1, since that setup allows withdrawals to be completed before any off-chain checks can run.

Yellow Team: Acknowledged.

The protocol does not maintain on-chain accounting. All accounting is performed off-chain based on on-chain events. Users should be aware that they cannot track or view their balances directly on-chain and must use the protocol UI.

This is an informational issue intended to highlight the protocol design for readers and users. No changes are required.

Yellow Team: Acknowledged.

The README in the Nitewatch repository states: " The **Event Daemon** waits for the outcome (WithdrawFinalized or WithdrawRejected), fires an internal event, and NeoDAX debits the balance upon successful confirmation."

However, the custody contract only emits the WithdrawFinalized event with a bool success parameter. It emits true for successful withdrawals and false for rejected ones, and does not emit a WithdrawRejected event.

Consider updating the README to reflect the actual contract behavior. Also, ensure that off-chain listeners do not rely on a WithdrawRejected event and instead correctly differentiate WithdrawFinalized events based on the boolean success value.

Yellow Team: Acknowledged.

The _executeWithdrawal function clears storage variables after performing an unsafe external call. While all important functions use the nonReentrant modifier and the request.finalized flag is set to true before the external call, preventing possible reentrancy, it is recommended to update all state variables before making an external call to an arbitrary address.

Update state variables before making external calls as a best practice.

Yellow Team: Acknowledged.