Enforce fair FIFO queue ordering on-chain

[m-Peter]

Closes #25

FLOW-10: FIFO Queue Is Not Enforced on-Chain yet Costs O(n) to Maintain

startProcessing() & completeProcessing() currently accept the request ID to process next. This is the API which allows the authorized COA to by-pass FIFO fairness on-chain, by ignoring what is the next request ID in line on pendingRequestIds array.

To avoid this, we remove the requestId parameter from these 2 methods, and simply rely on the contract's internal state, to pick up the next request ID in line, for processing. This allows us to guarantee FIFO fairness on-chain. We also introduce a more optimized queue data structures (mapping-based), to avoid the high gas costs of maintaining the FIFO order on-chain.

[claude]

Code Review: Enforce Fair FIFO Queue Ordering On-Chain

Summary

This PR successfully addresses the FIFO fairness issue by removing the requestId parameter from startProcessing() and completeProcessing(), forcing the COA to process requests strictly in queue order. The implementation replaces the array-based queue with a mapping-based queue to reduce gas costs while maintaining FIFO guarantees.

✅ Strengths

1. Security Enhancement: Removing the requestId parameter from processing functions prevents the authorized COA from bypassing FIFO fairness, which is critical for DeFi applications.

2. Gas Optimization: The new mapping-based queue (requestsQueue) with head/tail pointers is more gas-efficient than the previous array-based implementation for enqueue/dequeue operations (O(1) instead of O(n) shifts).

3. Validation: Both Cadence functions now decode and validate the returned requestId from the EVM calls, ensuring cross-VM consistency.

4. Test Coverage: Comprehensive test updates cover edge cases including empty queue scenarios, sequential processing, and random cancellations.

🔴 Critical Issues

1. Bug in _dropQueuedRequest() - Out-of-Bounds Access

Location: solidity/src/FlowYieldVaultsRequests.sol:1779-1797

Problem: When i == _requestsQueueTail - 1 (last iteration), accessing requestsQueue[i+1] reads from requestsQueue[_requestsQueueTail], which is the position after the tail. This could read uninitialized/deleted data and leave incorrect data in the second-to-last position.

Impact: Corrupted queue state when canceling requests, potentially breaking FIFO ordering.

Recommendation: Add bounds checking - if (shiftQueue && i < _requestsQueueTail - 1)

2. Inefficiency in _dropQueuedRequest() - Unnecessary Tail Decrement

Location: solidity/src/FlowYieldVaultsRequests.sol:1796

Problem: The function unconditionally decrements _requestsQueueTail even if the requestId was not found in the queue. This could cause incorrect queue length and lead to data loss.

Recommendation: Only decrement if the request was actually found - if (shiftQueue) { _requestsQueueTail -= 1; }

3. Missing NatSpec Documentation

Location: solidity/src/FlowYieldVaultsRequests.sol:275

Issue: Empty @notice tag for EmptyRequestsQueue error provides no information.

Recommendation: Add meaningful description like "Reverted when attempting to peek/dequeue from an empty requests queue"

⚠️ Important Considerations

4. Gas Cost Concern: O(n) Cancellations Still Required

While the new queue structure optimizes enqueue/dequeue to O(1), the _dropQueuedRequest() function still requires O(n) iteration and shifting to maintain FIFO order during cancellations. This is identical to the previous implementations cost for mid-queue removals.

Trade-offs:

• PRO: O(1) enqueue/dequeue (common operations)
• CON: O(n) cancellations (less common but still possible)

Consideration: Document this trade-off clearly. Users should understand that canceling mid-queue requests remains gas-intensive.

5. Error Message Inconsistency in Cadence

Location: cadence/contracts/FlowYieldVaultsEVM.cdc:1049-1055 and 1163

The error messages say "expected X but got Y" where the variable positions are swapped. Should say "expected requestId, but got processedRequestId".

Impact: Low - confusing debug messages but no functional impact.

🟡 Minor Issues

6. Inconsistent Spacing in Loop

Location: solidity/src/FlowYieldVaultsRequests.sol:1224

Line 1224 uses for (uint256 i = 0; i < size;) without space before {, while most other loops use spaces.

7. Unused Return Value in Tests

Several test functions dont capture the returned requestId from startProcessing() and completeProcessing(). While not strictly necessary, capturing these values could enable additional assertions.

📋 Security Considerations

1. Reentrancy Protection: Maintained via nonReentrant modifier ✅
2. Access Control: onlyAuthorizedCOA correctly enforced ✅
3. State Consistency: Two-phase commit pattern preserved ✅
4. Queue Integrity: Requires fixing the _dropQueuedRequest() bugs ⚠️

🧪 Test Coverage

The PR includes excellent test coverage:

• Empty queue scenarios ✅
• Sequential processing enforcement ✅
• Random order cancellations ✅
• FIFO ordering verification ✅
• Multi-user request handling ✅

Recommendation: Add test case for dropping last queue element (to expose the out-of-bounds bug).

📊 Performance Impact

Before: O(n) shifts to maintain FIFO order in array
After:

• Enqueue/Dequeue: O(1) ✅
• Cancellations: O(n) (unchanged)
• Queue traversal: O(n) (unchanged but now constructs array on-demand)

The change is a net positive for the common path (processing requests).

✨ Recommendations Summary

Must Fix Before Merge:

1. Fix out-of-bounds access in _dropQueuedRequest() (line 1788)
2. Only decrement tail if request found in _dropQueuedRequest() (line 1796)
3. Add NatSpec documentation for EmptyRequestsQueue error

Should Fix:
4. Correct error message variable ordering in Cadence validation (lines 1050, 1163)

Nice to Have:
5. Consistent code formatting in loops
6. Document the O(n) cancellation cost trade-off in comments/docs
7. Add test case for dropping last queue element

🎯 Conclusion

This PR successfully enforces FIFO ordering on-chain and provides meaningful gas optimizations for the primary request processing path. However, the critical bug in _dropQueuedRequest() must be fixed before merging to prevent queue corruption. Once addressed, this will be a solid improvement to the protocols fairness guarantees.

Recommendation: Request changes to fix critical issues, then approve.

---

Review generated with Claude Code