Skip to content

fix(FLOW-10): Enforce fair FIFO queue ordering on-chain #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m-Peter wants to merge 1 commit into
base: main
Choose a base branch
from
+425 −97
Open

fix(FLOW-10): Enforce fair FIFO queue ordering on-chain #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 146 additions & 52 deletions solidity/src/FlowYieldVaultsRequests.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,16 +170,20 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
/// @notice All requests indexed by request ID
mapping(uint256 => Request) public requests;

/// @notice Array of pending request IDs awaiting processing (FIFO order)
uint256[] public pendingRequestIds;

/// @notice Index of request ID in global pending array (for O(1) lookup)
mapping(uint256 => uint256) private _requestIndexInGlobalArray;

/// @notice Index of yieldVaultId in user's yieldVaultsByUser array (for O(1) removal)
/// @dev Internal visibility allows test helpers to properly initialize state
mapping(address => mapping(uint64 => uint256)) internal _yieldVaultIndexInUserArray;

/// @notice Mapping of queued request IDs awaiting processing (FIFO order)
mapping(uint256 => uint256) private _requestsQueue;

/// @notice Pointer to the current head in _requestsQueue. Denotes the next request to be processed
uint256 private _requestsQueueHead = 1;

/// @notice Pointer to the current tail in _requestsQueue. Points to the next available
/// slot — i.e., one past the last enqueued request.
uint256 private _requestsQueueTail = 1;

// ============================================
// Errors
// ============================================
Expand Down Expand Up @@ -282,6 +286,12 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
/// @notice No refund available for the specified token
error NoRefundAvailable(address token);

/// @notice Invalid dequeue operation on an empty requests queue
error EmptyRequestsQueue();

/// @notice Processed request does not match the head of requestsQueue
error RequestProcessOutOfOrder(uint256 expectedId, uint256 processedId);

// ============================================
// Events
// ============================================
Expand Down Expand Up @@ -842,7 +852,8 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
if (userPendingRequestCount[request.user] > 0) {
userPendingRequestCount[request.user]--;
}
_removePendingRequest(requestId);
_removeUserPendingRequest(requestId);
_dropQueuedRequest(requestId);

// === REFUND HANDLING (pull pattern) ===
// For CREATE/DEPOSIT requests, move funds from pendingUserBalances to claimableRefunds
Expand Down Expand Up @@ -911,6 +922,9 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
* @notice Processes a batch of PENDING requests.
* @dev For successful requests, marks them as PROCESSING.
* For rejected requests, marks them as FAILED.
* Requests are classified as successful/rejected based on validation
* logic that is performed on Cadence side, and not on the authorized
* COA's discretion.
* Single-request processing is supported by passing one request id in
* successfulRequestIds and an empty rejectedRequestIds array.
* @param successfulRequestIds The request ids to start processing (PENDING -> PROCESSING)
Expand All @@ -920,6 +934,38 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
uint256[] calldata successfulRequestIds,
uint256[] calldata rejectedRequestIds
) external onlyAuthorizedCOA nonReentrant {
uint256 totalRequests = successfulRequestIds.length + rejectedRequestIds.length;

uint256 j = 0;
uint256 k = 0;
for (uint256 i = 0; i < totalRequests; i++) {
uint256 requestId = _requestsQueue[_requestsQueueHead+i];
uint256 reqId;
if (j < successfulRequestIds.length) {
reqId = successfulRequestIds[j];
Request storage request = requests[reqId];

// === VALIDATION ===
if (request.id != reqId) revert RequestNotFound();
if (request.status != RequestStatus.PENDING)
revert InvalidRequestState();

if (reqId == requestId) {
j++;
continue;
}
}

if (k < rejectedRequestIds.length) {
reqId = rejectedRequestIds[k];
if (reqId == requestId) {
k++;
continue;
}
}

revert RequestProcessOutOfOrder(requestId, reqId);
}
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undocumented ordering constraint + misleading error payload

The validation loop enforces that both successfulRequestIds and rejectedRequestIds must be in FIFO queue order — i.e., they must individually form sorted subsequences of the queue. If either array lists requests out of queue order, the function reverts here.

This constraint is not documented anywhere in the NatSpec. The Cadence preprocessRequests must preserve FIFO order in each output array; if it ever groups or sorts by any other criterion, all batch transactions will spuriously revert.

The error payload is also confusing: RequestProcessOutOfOrder(requestId, reqId) where reqId is the last non-matching ID tried (either from successfulRequestIds[j] or rejectedRequestIds[k] — whichever was tested last), not the "processedId" the error name implies. A caller decoding the error will see a value that has no obvious relationship to the request being processed.

Suggested fixes:

  1. Add @dev NatSpec: both arrays must be in ascending FIFO queue order (i.e., must form ordered subsequences of the current queue starting at head).
  2. For the error, capture which input array and index caused the mismatch, or simplify to just emit the expected queue-head value.


// === REJECTED REQUESTS ===
_dropRequestsInternal(rejectedRequestIds);
Expand Down Expand Up @@ -1071,12 +1117,21 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
/// @notice Gets the count of pending requests
/// @return Number of pending requests
function getPendingRequestCount() external view returns (uint256) {
return pendingRequestIds.length;
return _requestsQueueLength();
}

/// @notice Gets all pending request IDs
/// @return Array of pending request IDs
function getPendingRequestIds() external view returns (uint256[] memory) {
uint256[] memory pendingRequestIds = new uint256[](_requestsQueueLength());
uint256 arrayIndex = 0;
for (uint256 i = _requestsQueueHead; i < _requestsQueueTail;) {
pendingRequestIds[arrayIndex] = _requestsQueue[i];
unchecked {
++arrayIndex;
++i;
}
}
return pendingRequestIds;
}

Expand Down Expand Up @@ -1115,7 +1170,7 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
string[] memory strategyIdentifiers
)
{
if (startIndex >= pendingRequestIds.length) {
if (startIndex >= _requestsQueueLength()) {
return (
new uint256[](0),
new address[](0),
Expand All @@ -1131,7 +1186,7 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
);
}

uint256 remaining = pendingRequestIds.length - startIndex;
uint256 remaining = _requestsQueueLength() - startIndex;
uint256 size = count == 0
? remaining
: (count < remaining ? count : remaining);
Expand All @@ -1148,8 +1203,8 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
vaultIdentifiers = new string[](size);
strategyIdentifiers = new string[](size);

for (uint256 i = 0; i < size; ) {
Request memory req = requests[pendingRequestIds[startIndex + i]];
for (uint256 i = 0; i < size;) {
Request memory req = requests[_requestsQueue[_requestsQueueHead + startIndex + i]];
ids[i] = req.id;
users[i] = req.user;
requestTypes[i] = uint8(req.requestType);
Expand Down Expand Up @@ -1412,7 +1467,8 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
}

// Remove from pending queues (both global and user-specific)
_removePendingRequest(requestId);
_removeUserPendingRequest(requestId);
_dropQueuedRequest(requestId);
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent skip + missing _dropQueuedRequest call creates queue/state divergence

_dropRequestsInternal only calls _dropQueuedRequest when request.status == RequestStatus.PENDING. If that guard is false, the request remains in _requestsQueue (the pointer-based queue is not updated) while the request's state is unchanged. The caller (startProcessingBatch) has already verified these IDs occupy specific queue slots via the validation loop — so a silent skip would leave a stale entry in the queue, stalling all subsequent dequeues.

In practice, any request in _requestsQueue should always be PENDING (only PENDING requests are enqueued; they exit via dequeue/drop). But given that the validation loop immediately before already confirmed the ID is in the queue, the guard mismatch should be an explicit revert rather than a silent skip, so bugs surface immediately rather than corrupting queue state silently.


emit RequestProcessed(
requestId,
Expand Down Expand Up @@ -1457,11 +1513,6 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
function _startProcessingInternal(uint256 requestId) internal {
Request storage request = requests[requestId];

// === VALIDATION ===
if (request.id != requestId) revert RequestNotFound();
if (request.status != RequestStatus.PENDING)
revert InvalidRequestState();

// === TRANSITION TO PROCESSING ===
// This prevents cancellation and ensures atomicity with completeProcessing
request.status = RequestStatus.PROCESSING;
Expand Down Expand Up @@ -1517,7 +1568,9 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
if (userPendingRequestCount[request.user] > 0) {
userPendingRequestCount[request.user]--;
}
_removePendingRequest(requestId);
_removeUserPendingRequest(requestId);
uint256 reqId = _dequeueRequest();
if (reqId != requestId) revert RequestProcessOutOfOrder(reqId, requestId);
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestProcessOutOfOrder parameter order is reversed vs. line 967

Here the error is (reqId, requestId) — (dequeued_head, caller_requested) — which reads "expected reqId, got requestId". That's semantically clear.

At line 967, the same error is emitted as (requestId, reqId) — (queue_slot_value, last_tried_input). The second argument there is not the request being "processed"; it's whichever ID was last compared before failing.

The two call sites disagree on which argument is "expected" and which is "processed", making off-chain monitoring and incident investigation harder. Document the parameter semantics in the error definition, or pick a single consistent convention.


emit RequestProcessed(
requestId,
Expand Down Expand Up @@ -1758,8 +1811,7 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
});

// Add to global pending queue with index tracking for O(1) lookup
_requestIndexInGlobalArray[requestId] = pendingRequestIds.length;
pendingRequestIds.push(requestId);
_enqueueRequest(requestId);
userPendingRequestCount[msg.sender]++;

// Add to user's pending array with index tracking for O(1) removal
Expand Down Expand Up @@ -1795,40 +1847,16 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
}

/**
* @dev Removes a request from all pending queues while preserving request history.
* Uses two different removal strategies:
* - Global array: Shift elements to maintain FIFO order (O(n) but necessary for fair processing)
* - User array: Swap-and-pop for O(1) removal (order doesn't affect processing)
* @dev Removes a request from the user pending requests mapping while preserving request history.
* Uses the following removal strategy:
* - Swap-and-pop for O(1) removal (order doesn't affect processing)
*
* The request data remains in the `requests` mapping for historical queries;
* this function only removes it from the pending queues.
* @param requestId The request ID to remove from pending queues.
* This function only removes it from the user pending requests mapping.
* @param requestId The request ID to remove from the user pending requests mapping.
*/
function _removePendingRequest(uint256 requestId) internal {
// === GLOBAL PENDING ARRAY REMOVAL ===
// Uses O(1) lookup + O(n) shift to maintain FIFO order
// FIFO order is critical for DeFi fairness - requests must be processed in submission order
uint256 indexInGlobal = _requestIndexInGlobalArray[requestId];
uint256 globalLength = pendingRequestIds.length;

// Safety check: verify element exists at expected index
if (globalLength > 0 && indexInGlobal < globalLength && pendingRequestIds[indexInGlobal] == requestId) {
// Shift all subsequent elements left to maintain FIFO order
for (uint256 j = indexInGlobal; j < globalLength - 1; ) {
pendingRequestIds[j] = pendingRequestIds[j + 1];
// Update index mapping for each shifted element
_requestIndexInGlobalArray[pendingRequestIds[j]] = j;
unchecked {
++j;
}
}
// Remove the last element (now duplicated or the one to remove)
pendingRequestIds.pop();
// Clean up index mapping
delete _requestIndexInGlobalArray[requestId];
}

// === USER PENDING ARRAY REMOVAL ===
function _removeUserPendingRequest(uint256 requestId) internal {
// === USER PENDING REQUESTS ARRAY REMOVAL ===
// Uses swap-and-pop for O(1) removal (order doesn't affect FIFO processing)
address user = requests[requestId].user;
uint256[] storage userPendingIds = pendingRequestIdsByUser[user];
Expand All @@ -1850,4 +1878,70 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
delete _requestIndexInUserArray[requestId];
}
}

/**
* @dev Enqueues a request in the requestsQueue and shifts the queue's tail pointer.
*
* @param requestId The request ID to enqueue in the pending requests queue.
*/
function _enqueueRequest(uint256 requestId) internal {
_requestsQueue[_requestsQueueTail] = requestId;
_requestsQueueTail += 1;
}

/**
* @dev Dequeues the head of requestsQueue and shifts the queue's head pointer.
*
* @return The request ID that was dequeued.
*/
function _dequeueRequest() internal returns (uint256) {
if (_requestsQueueLength() == 0) revert EmptyRequestsQueue();

uint256 requestId = _requestsQueue[_requestsQueueHead];

delete _requestsQueue[_requestsQueueHead];
_requestsQueueHead += 1;

return requestId;
}

/**
* @dev Drops a request from the requestsQueue.
* O(n) operation — scans from the removed element to the tail and shifts
* the queue to all subsequent elements left to maintain FIFO order.
*
* @param requestId The request ID to remove from the pending requests queue.
*/
function _dropQueuedRequest(uint256 requestId) internal {
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejection/cancellation path is still O(n) — batch rejection is O(k×n)

The PR description claims "both enqueue & dequeue operations are now O(1)", which is accurate for the happy path. However _dropQueuedRequest — called once per rejected request in _dropRequestsInternal and also on cancelRequest — is O(queue length) due to the element-shift loop. Processing a batch with k rejections against a queue of length n is therefore O(k×n).

For workloads where a large fraction of requests are rejected (e.g., spam bursts), this can exceed the original O(n) shift cost of the array-based approach. The docstring already acknowledges this ("O(n) operation"), but the PR summary should be updated to avoid overstating the improvement.

bool requestFound = false;
for (uint256 i = _requestsQueueHead; i < _requestsQueueTail;) {
if (_requestsQueue[i] == requestId) {
requestFound = true;
}

// Shift the matching request to the queue's tail, then delete it
if (requestFound && (i + 1 < _requestsQueueTail)) {
_requestsQueue[i] = _requestsQueue[i + 1];
} else if (requestFound) {
delete _requestsQueue[i];
}

unchecked {
++i;
}
}

// Decrement the queue tail only if the given requestId was found
if (!requestFound) revert RequestNotFound();
_requestsQueueTail -= 1;
}

/**
* @dev Counts the total number of pending requests in the requestsQueue.
*
* @return The current requestsQueue length.
*/
function _requestsQueueLength() internal view returns (uint256) {
return _requestsQueueTail - _requestsQueueHead;
}
}
Loading
Loading