FLO-29: Per-Position Reentrancy Lock Does Not Protect Shared Pool State During External Callbacks

[liobrasil]

Severity: Low

Files Affected

• cadence/contracts/FlowALPv1.cdc

Description

The reentrancy guard (_lockPosition/ _unlockPosition, lines 1572–1583) locks by position ID, not at the pool level. While position A is locked and making external calls (to oracle, DEX swapper, sink, or source), any other position B can freely operate on shared mutable pool state — reserves, TokenState balances, interest indices/rates, and deposit capacity.

Recommendation

If the intent is to protect shared state during external callbacks, consider either (a) a pool-level lock that prevents all position operations during any external call, or (b) locking the affected TokenState and reserve vaults in addition to the position. A lighter alternative is to snapshot shared state before the external call and validate it hasn't changed after the call returns, reverting if it has.

---

Parent Issue: #209

[holyfuchs]

While the reentrancy guard is scoped per position ID, we did not identify a concrete attack vector that would allow this to be exploited to manipulate shared pool state. As such, this is a theoretical concern rather than a practical vulnerability.

That said, we have not exhaustively analyzed all possible multi-position interaction patterns and may review this further. Introducing a pool-level lock would also restrict legitimate interactions involving multiple positions within the same transaction, so there are some design tradeoffs to consider.

[jordanschalm]

I'm in favour of adding the pool-level lock.

Introducing a pool-level lock would also restrict legitimate interactions involving multiple positions within the same transaction

Most legitimate transactions which operate on multiple positions in one transaction would still function with a pool-scoped lock. Any sequence of operations -- where each operation finishes before the next starts -- on multiple positions will work fine. The only thing that won't work is when a position operation uses a dependency (Sink, Source, Swapper, etc) that then calls back into the Pool. I don't think there is a clear need for supporting this kind of pattern.

That said, we have not exhaustively analyzed all possible multi-position interaction patterns

For this kind of high-assurance software project, we need to provide affirmative evidence that the software behaves as expected under all conditions. If we don't know that it's safe, we need to assume it's not. I think the audit suggestion is appropriate.