fix(universal-router-sdk): mask ALLOW_REVERT_FLAG bit in commandParser - #700
Open
gomesalexandre wants to merge 2 commits into
Open
gomesalexandre wants to merge 2 commits into
gomesalexandre wants to merge 2 commits into
Conversation
GenericCommandParser.getCommands() parsed each command byte straight into a CommandType enum value without stripping the ALLOW_REVERT_FLAG bit (0x80) that routerCommands.ts ORs onto revertible commands like EXECUTE_SUB_PLAN. Real calldata containing such a command (e.g. a sub-plan encoded as 0x21 | 0x80 = 0xa1) has no matching entry in commandDefinition, so parse() throws TypeError reading 'parser' off undefined. Mask the flag off before the enum cast, using the newly-exported ALLOW_REVERT_FLAG constant instead of a magic number. Also fixes the same stale-mask bug in test/utils/uniswapData.ts's parseCommands helper, which used `& 0x3f` - a mask that already silently zeroed out ACROSS_V4_DEPOSIT_V3 (0x40) on its own terms. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by inspection, no filed issue.
GenericCommandParser.getCommandsparsed each command byte straight into aCommandTypeenum value without stripping theALLOW_REVERT_FLAGbit (0x80) thatrouterCommands.tsORs onto revertible commands likeEXECUTE_SUB_PLAN. Realcalldata containing such a command (e.g. a sub-plan encoded as
0x21 | 0x80 = 0xa1) has no matching entry incommandDefinition, soparse()throwsTypeError: Cannot read properties of undefined (reading 'parser').This mirrors on-chain dispatch semantics:
Commands.soldefinesFLAG_ALLOW_REVERT = 0x80andCOMMAND_TYPE_MASK = 0x7f, andDispatcher.sol:57dispatches via
uint8(commandType & Commands.COMMAND_TYPE_MASK). The parser shouldmask the same way the contract does before treating the byte as a command type -
this PR makes it do that.
Fix
ALLOW_REVERT_FLAGfromrouterCommands.ts(was previously unexported;no other package in the monorepo depends on it, and it's not re-exported from
src/index.ts, so nothing new enters the public API surface).commandParser.ts, mask it off before the enum cast:parseInt(byte, 16) & ~ALLOW_REVERT_FLAG.test/utils/uniswapData.ts'sparseCommandsused astale
& 0x3fmask, which already silently zeroed outACROSS_V4_DEPOSIT_V3 = 0x40on its own terms (independent of this fix -0x40 & 0x3f === 0). Fixed touse the same correct mask.
EXECUTE_SUB_PLANis the only command type the SDK's own encoder can ever set thisflag on (
REVERTIBLE_COMMANDSgates it,addCommandthrows forallowRevertonanything else), so the new test covers the one case the SDK itself can produce -
masking every byte unconditionally is still correct and contract-faithful for
arbitrary/foreign calldata, matching
Dispatcher.sol's own unconditional mask.Parse output (
UniversalRouterCommand) intentionally doesn't surface whether theflag was set on a given command - that field never existed in the type, and adding
it would be a separate API addition, not part of this fix.
Testing
New regression test in
commandParser.test.ts, using the realRoutePlanner().addSubPlan()codepath (not a hand-crafted byte) - builds aWRAP_ETH sub-plan, asserts the encoded command byte is literally
0xa1, thenasserts
CommandParser.parseCalldataresolves it back toCommandType.EXECUTE_SUB_PLANwithout throwing.Guard-validated: reverted just the
getCommandsmasking, confirmed the new testfails with the exact TypeError described above, restored, confirmed 22/22 pass in
that file. Full
hardhat testsuite: 477 passing, 1 failing - the failure isuniswapTrades.test.ts's mainnet-forkbefore allhook hitting a 403 from a publicRPC (no
FORK_URLconfigured in this environment), isolated as pre-existing andunrelated via
git stashcomparison (identical failure with zero diff applied).tsc --noEmitand lint both clean. Changeset added (patch,@uniswap/universal-router-sdk).