fix: L04 Bridge Safety Check Can Revert Instead Of Returning False, Disabling HyperEVM Fallback On Fills - #1541
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4f854f12d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uint64 currentBridgeBalance = spotBalance(bridgeAddress, erc20CoreIndex); | ||
| // Deliberately not `toSystemAddress`: this is a predicate, and an unlinked token's system address simply | ||
| // holds nothing, so it evaluates to false instead of reverting a fill that has a HyperEVM fallback. | ||
| uint64 currentBridgeBalance = spotBalance(toSystemAddressNoRevert(erc20CoreIndex), erc20CoreIndex); |
There was a problem hiding this comment.
When a token's cached configuration remains usable after its live evmContract link is removed, the derived bridge account can retain enough Core balance for this call to return true even though the token is no longer bridgeable. The handler then skips its HyperEVM fallback and transferERC20EVMToCore() calls the reverting toSystemAddress(), so the fill still fails—the exact failure this change intends to prevent. Use a non-reverting live linkage lookup and return false when the link is absent or cannot be resolved, rather than treating the derived account's balance as proof of bridgeability.
Useful? React with 👍 / 👎.
| // beyond that domain (spotBalance's uint64 also carries encoded outcome asset ids) can't be resolved. | ||
| if (erc20CoreIndex > type(uint32).max) revert TokenNotBridgeable(erc20CoreIndex); | ||
| uint32 index = uint32(erc20CoreIndex); | ||
| if (isHype(index)) return HYPE_SYSTEM_ADDRESS; // must precede the linkage check: HYPE has no evmContract |
There was a problem hiding this comment.
isHype() function is not longer being used, lets remove it
grasphoper
left a comment
grasphoper
left a comment
There was a problem hiding this comment.
Can a cleaner solution to this be returning an extra bool from toSystemAddress?
function toSystemAddress(uint64 erc20CoreIndex) internal view returns (address addr, bool success)
Then isCoreAmountSafeToBridge can return false if toSystemAddress returns false.
We can then have toSystemAddressStrict or something that will call toSystemAddress internally and revert if we want revert functionality in other places.
Or just make current toSystemAddressNoRevert return a bool too. Prevent duplication of functionality between toSystemAddressNoRevert and toSystemAddress
Before #1530 we never reverted on a similar call so instead of reverting, the callers could use a bool-returning function and just ignore the bool too 🤷
3 participants
HyperCoreLib.isCoreAmountSafeToBridge reports whether the asset bridge holds enough to cover a transfer, and callers use a false answer to pay the user on HyperEVM instead of bridging. This matters most on the Across fill path: HyperliquidDepositHandler is an AcrossMessageHandler whose handleV3AcrossMessage is invoked by the SpokePool after it has already transferred the tokens to the handler, so something must be done with funds that have arrived. _depositToHypercore runs the check before making any state changes or withdrawals and, on false, transfers the tokens to the user on HyperEVM and emits FallbackToHyperEVM, allowing the fill to succeed. HyperCoreFlowExecutor applies the same pattern through _fallbackHyperEVMFlow, as does its swap path.
The check resolved the bridge address through toAssetBridgeAddress, plain arithmetic that could not fail, so it always returned true or false. Pull request #1530 repoints line 418 to toSystemAddress, which reverts when the core index exceeds uint32, when the token's Core entry has no linked HyperEVM contract, or when the tokenInfo precompile call fails. Reverting is correct where toSystemAddress resolves a destination that funds are about to be sent to, since sending to an unlinked token would strand them. However, inherited by a check whose purpose is to return an answer, it changes what happens on the fill path: for a token with no linked HyperEVM contract the handler now reverts, so the SpokePool fill reverts and the intent goes unfilled. Because the condition is permanent rather than transient, every relayer fails identically and the user waits for the origin-chain refund instead of receiving the tokens on HyperEVM.
The handler's other entry point, depositToHypercore, performs its own safeTransferFrom, so a revert there simply unwinds and leaves the caller holding the tokens, and an unlinked token is a misconfiguration that arguably should fail loudly rather than silently divert users to HyperEVM.
Consider giving the check a non-reverting way to resolve the address, for example a variant returning a success flag alongside it, and having isCoreAmountSafeToBridge return false when resolution fails, so the fill path keeps its fallback; a token with no HyperEVM side is genuinely not safe to bridge. toSystemAddress should keep reverting for callers that resolve an address in order to send to it. If failing loudly on an unlinked token is intended, consider applying that only to depositToHypercore, where a revert unwinds the caller's own transfer and leaves them holding their tokens, while keeping the HyperEVM fallback on handleV3AcrossMessage, where a revert makes the SpokePool fill fail and the intent go unfilled.