fix: revert when msg.value does not cover a native fill - #132
Open
dev-aly3n wants to merge 1 commit into
Open
Conversation
_fill's native branch sends outputAmount to the recipient, but nothing requires msg.value to cover it. _refundNativeExcess handles only the excess case, so a shortfall is paid out of address(this).balance. The check is placed in _refundNativeExcess rather than in _fill because fillOrderOutputs accumulates totalNativeSent across outputs and settles once; a per-output check would pass on each individual output while the total overdrew. Both entry points already funnel through this helper. Adds a batch test for the cumulative overdraw case and one asserting a correctly funded fill still refunds its excess and leaves a residual balance untouched.
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
1 participant
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.
Description
_fill's native branch sendsoutputAmountto the recipient, but nothing requiresmsg.valueto cover it._refundNativeExcesshandles only the excess case, so a shortfall is paid out ofaddress(this).balance.Not exploitable today. The settler holds no ether and has no
receive()/fallback(), soAddress.sendValuereverts. The concern is that this is safe by circumstance rather than by construction: any wei that reaches the contract (SELFDESTRUCT force-feed, a block-reward/withdrawal recipient, a future residue) would become permissionlessly sweepable via a self-order with a native output andmsg.value = 0.This is distinct from OpenZeppelin May-2026 L-01, which fixed the refund leg; that fix is present and correct. This adds the missing positive coverage check.
The check is placed in
_refundNativeExcessrather than in_filldeliberately:fillOrderOutputsaccumulatestotalNativeSentacross outputs and refunds once, so a per-output check would pass on each individual output while the total overdrew. Both entry points already funnel through this helper.Includes three tests: a single output underfunded, a batch where the sum overdraws but no individual output does, and the funded path still refunding its excess correctly.
Related Issues
msg.valuedoes not cover a native fill openintentsframework/oif-contracts#194Third-Party Integration Checklist
N/A. This PR does not integrate with any external protocol: no new dependencies, no interfaces copied.
Additional Notes
Note on the batch test: with a zero contract balance, a batch overdraw already reverts inside
_fillviaAddress.sendValue(InsufficientBalance), which masks the missing check. The new batch test therefore seeds the settler with a residual balance so the overdraw actually reaches_refundNativeExcess, which is the condition the check exists for.Gas snapshots are not touched here: this repo's committed snapshots are already stale relative to a clean
forge testrun, so regenerating them would mix unrelated drift into the diff.