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 3 commits into
base: main
Choose a base branch
from
+563 −99
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
3 commits
Select commit Hold shift + click to select a range
0004434
Enforce fair FIFO queue ordering on-chain
m-Peter Mar 10, 2026
7001cdf
Validate requests state in startProcessingBatch()
m-Peter Mar 30, 2026
09a4fa2
Merge origin/main into codex/pr46-conflicts-20260407
liobrasil Apr 7, 2026
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
209 changes: 162 additions & 47 deletions solidity/src/FlowYieldVaultsRequests.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,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 @@ -309,6 +313,15 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
/// @notice The requested recovery amount exceeds the available excess amount
error InsufficientRecoveryAmount(uint256 available, uint256 requested);

/// @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);

/// @notice Request is not included in requestsQueue
error RequestNotInQueue(uint256 requestId);

// ============================================
// Events
// ============================================
Expand Down Expand Up @@ -901,7 +914,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 @@ -971,6 +985,10 @@ 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.
* Both arrays containing the request IDs must be in ascending FIFO queue order.
* 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 @@ -980,7 +998,50 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
uint256[] calldata successfulRequestIds,
uint256[] calldata rejectedRequestIds
) external onlyAuthorizedCOA nonReentrant {
uint256 j = 0;
uint256 k = 0;
// Validate that the given IDs for successful/rejected requests,
// are according to the FIFO queue order.
uint256 totalRequests = successfulRequestIds.length + rejectedRequestIds.length;
for (uint256 i = 0; i < totalRequests; i++) {
uint256 requestId;
// If reqId is 0, it means we went over the boundaries of
// _requestsQueue.
uint256 reqId = _requestsQueue[_requestsQueueHead+i];
Copy link
Copy Markdown

@claude claude bot Apr 7, 2026

Choose a reason for hiding this comment

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

The reqId == 0 guard fires only inside the if (j < successfulRequestIds.length) and if (k < rejectedRequestIds.length) branches. When both arrays are exhausted midway through the loop (impossible in practice because totalRequests = j_max + k_max keeps j + k == i+1 at every iteration), a zero value at a queue slot would silently skip past the guard and reach the RequestProcessOutOfOrder revert instead.

This is unreachable today, but worth noting that the "went over the boundaries" invariant is only protected within each branch. The loop invariant j + k == i+1 (one match per iteration) prevents this from being exploitable, but it is worth a clarifying comment.

if (j < successfulRequestIds.length) {
requestId = successfulRequestIds[j];
if (reqId == 0) revert RequestNotInQueue(requestId);
if (reqId == requestId) {
j++;
continue;
}
}

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

// === VALIDATION ===
Request storage request = requests[reqId];
if (request.status != RequestStatus.PENDING)
revert InvalidRequestState();
Copy link
Copy Markdown

@claude claude bot Apr 7, 2026

Choose a reason for hiding this comment

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

This InvalidRequestState check is dead code. The _requestsQueue mapping only ever holds IDs for requests that are currently PENDING — every transition out of PENDING (_dequeueRequest, _dropQueuedRequest) removes the ID from the queue first. So reqId read from the queue is always PENDING here.

Leaving dead code in a hot code path is misleading: it suggests to future readers that non-PENDING entries can appear in the queue, which they cannot. Consider removing it (or replacing with a assert-style comment if you want to document the invariant).


// requestId currently holds the last-assigned candidate
// (from rejectedIds if both branches ran).
// Prefer the successful candidate for a clearer error.
uint256 candidateReqId = (j < successfulRequestIds.length)
? successfulRequestIds[j]
: requestId;
revert RequestProcessOutOfOrder(reqId, candidateReqId);
}

// First the rejected request IDs are dropped, so successful
// request IDs are contiguous at the head before dequeue
// === REJECTED REQUESTS ===
_dropRequestsInternal(rejectedRequestIds);

Expand Down Expand Up @@ -1176,12 +1237,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 @@ -1220,7 +1290,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 @@ -1236,7 +1306,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 @@ -1253,8 +1323,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 @@ -1516,7 +1586,8 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
}

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

emit RequestProcessed(
requestId,
Expand Down Expand Up @@ -1587,6 +1658,9 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
if (request.status != RequestStatus.PENDING)
revert InvalidRequestState();

uint256 reqId = _dequeueRequest();
if (reqId != requestId) revert RequestProcessOutOfOrder(reqId, requestId);

// === TRANSITION TO PROCESSING ===
// This prevents cancellation and ensures atomicity with completeProcessing
request.status = RequestStatus.PROCESSING;
Expand Down Expand Up @@ -1643,7 +1717,7 @@ contract FlowYieldVaultsRequests is ReentrancyGuard, Ownable2Step {
if (userPendingRequestCount[request.user] > 0) {
userPendingRequestCount[request.user]--;
}
_removePendingRequest(requestId);
_removeUserPendingRequest(requestId);

emit RequestProcessed(
requestId,
Expand Down Expand Up @@ -1909,8 +1983,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 @@ -1946,40 +2019,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 @@ -2001,4 +2050,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 31, 2026

Choose a reason for hiding this comment

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

O(n) regression for cancelRequest

_dropQueuedRequest does a linear scan-and-shift across the entire queue, replacing the former O(1) swap-and-pop (which used _requestIndexInGlobalArray). This is the correct trade-off to maintain FIFO order, but it means that every user-initiated cancelRequest now costs gas proportional to queue depth, not O(1).

With maxPendingRequestsPerUser = 10 and N active users the queue is bounded at 10 × N entries, so gas stays manageable at current scale. Worth noting explicitly in the migration notes so operators are aware that raising maxPendingRequestsPerUser amplifies cancellation cost.

Copy link
Copy Markdown
Collaborator Author

@m-Peter m-Peter Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

cancelRequest previously used _removePendingRequest(requestId);, which stated:

// === 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

So this was also O(n).

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
Copy link
Copy Markdown

@claude claude bot Apr 7, 2026

Choose a reason for hiding this comment

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

Inaccurate comment. The algorithm does not "shift the matching request to the tail." It shifts all elements after the match one slot to the left (closing the gap), and then deletes the now-duplicated last slot. A clearer description:

// Once found, shift each subsequent element one position left,
// then delete the last (now-duplicate) slot at tail-1.

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