fix: enforce ExactPrice plugin in on_buy_plugin and cap overpayment in buy()#120
Open
fix: enforce ExactPrice plugin in on_buy_plugin and cap overpayment in buy()#120
Conversation
…n buy() - Load and execute ExactPrice plugin in on_buy_plugin, matching the existing comment that states it should run before other buy plugins. - Cap payment to listing price in buy() and refund any excess to the buyer via BankMsg::Send, ensuring the seller never silently receives more than the listing price regardless of plugin configuration.
crucible-burnt
left a comment
There was a problem hiding this comment.
🔍 Crucible Security Review
Summary
This PR adds defense-in-depth overpayment protection in buy() and ensures the ExactPrice plugin is enforced in on_buy_plugin. These changes relate to marketplace payment handling security.
Security Assessment
- Risk Level: Low (improvements with no regressions)
✅ Positive Findings:
-
Overpayment Refund Defense (
sale.rs:60-75)- Caps payment to listing price regardless of ExactPrice plugin configuration
- Refunds excess to buyer automatically
- Uses
checked_subto prevent underflow - Good comment explaining the defense-in-depth rationale
-
ExactPrice Plugin Enforcement (
plugin.rs:382-414)- Now loads and runs ExactPrice plugin in
on_buy_plugin - Runs BEFORE AllowedCurrencies plugin
- Ensures consistent enforcement across all buy paths
- Now loads and runs ExactPrice plugin in
-
Refund Message Ordering
- Refund is added via
response.add_message()- confirm execution order is correct (refund should happen after successful purchase) - Consider: should refund fail the tx or proceed with capped payment?
- Refund is added via
-
Error Type
ContractError::InsufficientFundson line 63 is used for checked_sub error - semantically this is a math error, not insufficient funds. Minor but could confuse debugging.
Immunefi Pattern Check
- #65150 (Marketplace PendingSale expiry):
⚠️ Not addressed by this PR - this PR fixes overpayment handling but the core issue of expired PendingSale approvals allowing NFT theft appears to be addressed by PR #118 ("reclaim expired sales") which is already reviewed - Related but separate concern from the expiry bypass vulnerability
False Report Risk
- Low - the changes are straightforward and well-commented
- The explicit comment at line 60-64 explains the intentional behavior
Code Quality Notes
- No new tests visible in the diff - recommend adding tests for:
- Overpayment refund behavior
- ExactPrice plugin interaction with buy flow
- Consider edge case: what if refund
BankMsg::Sendfails?
Status
Security review complete. This PR improves payment handling security. Not a fix for #65150 directly, but complements PR #118. Recommend merge after confirming test coverage.
Automated security review by Crucible (2026-02-24)
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.
Summary
Defense-in-depth hardening for the asset contract buy flow.
Changes
1. Load ExactPrice plugin in
on_buy_plugin(plugin.rs)The existing comment states ExactPrice should run before other buy plugins, but it was not being loaded. Now it is loaded and executed first.
2. Cap payment to listing price in
buy()(sale.rs)Regardless of plugin configuration,
buy()now caps the payment to the listing price and refunds any excess to the buyer viaBankMsg::Send. The seller never silently receives more than the listing price.Testing
All 44 existing tests pass. No behavioral change for correctly-priced purchases.