C4 S-41 Remove ineffective delegatecall guard. Add reentry guard

[ScreamingHawk]

S-41 Missing onlyDelegatecall modifier on critical TrailsRouter entrypoints enables direct implementation calls and potential fund extraction

https://code4rena.com/audits/2025-11-sequence-transaction-rails/submissions/S-41

Issue: Some functions on TrailsRouter are missing onlyDelegatecall modifier.

Analysis: This is intentional as TrailsRouter is called by the TrailsRouterShim. Due to this requirement, the onlyDelegatecall protection is ineffective, as funds can be claimed by pullAndExecute functions. More problematic is reentry conditions.

Fix: Removes the onlyDelegatecall guard from TrailsRouter. Adds more effective nonReentrant guard.

NOTE: This change required the addition of some internal functions to prevent internal calls to multiple nonReentrant functions. The flow between router functions is messy and could do with a refactor. This PR only seeks to make minimal changes.

NOTE 2: Tests pass without changes to reentrant logic and the API appears to not have reentry requirement. This is worth confirming before merging.

[ScreamingHawk]

I'm not opposed to scrapping this addition of the reentry guard.

[ScreamingHawk]

BTW My assumption here is that the TrailsRouter is never expected to receive funds from one transaction (e.g. bridge) and utilise on them in another (e.g. delayed swap?) as these should be batched. There are a few way such a flow could be exploited already, but this change makes the problem worse 😅

[Agusx1211]

TrailsRouter is never expected to have funds at all, Sequence v3 already implements a reentrancy guard, so when this is called in delegatecalls it would be a reentrancy guard on top of another.

If somehow funds end up in TrailsRouter then those funds are gone, regardless if they can be taken in any shape or form.

I don't think the issue is really a big issue, and I do not think it is worth the changes. I also think that onlyDelegatecall is a bit weird, but I wouldn't remove it this late in the process (as it is really cheap anyway).

I vote NOT to merge, unless @ScreamingHawk can convince me otherwise.

[Agusx1211]

Removing the onlyDelegatecall modifier is a good idea, but this duplicates the reentrancy guard with the native one of Sequence v3

I don't think we should merge this, as it does not really fix a real exploit

Open to change my mind

[ScreamingHawk]

TrailsRouter is never expected to have funds at all

@Agusx1211 . This is incorrect. The Shim will call the Router, it pulls funds and executes from the Router's context.

[ScreamingHawk]

I know that statement alone should leads to some architecture changes but I feel we are too late in the game for that.

[Agusx1211]

I mean it is not expected to "keep" funds, sorry for the poorly phrased comment

[ScreamingHawk]

I think exposing sweep functions could be beneficial to help clean up any dust left in the router. Any pullAndExecute flow can leave unspent tokens. Having sweep available will help clean them up.

Otherwise these funds are still accessible via execute pullAndExecute in a roundabout way.

Don't mind dropping the reentry check, but I think it's a good hardening addition.

Edit: Just saw #73. That doesn't prevent build up by invalid direct transfers. Which an accessible sweep would help clear.

[ScreamingHawk]

Closing as per comments above. #73 should prevent build up with legitimate use cases. We can hack the use of pullAndExecute to retrieve funds sent in error.