fix(asset/ft): enforce maker compliance at DEX settlement#138
Open
metalarm10 wants to merge 5 commits into
Open
fix(asset/ft): enforce maker compliance at DEX settlement#138metalarm10 wants to merge 5 commits into
metalarm10 wants to merge 5 commits into
Conversation
…ubtract DEXLocked
ysv
requested changes
Jun 12, 2026
| "to", send.ToAddress.String(), | ||
| "coin", send.Coin.String(), | ||
| ) | ||
| if err := k.validateDEXSettlementSend(ctx, send); err != nil { |
Contributor
There was a problem hiding this comment.
If I understood your fix correctly - you want to add assertion of freezing, whitelisting etc. for maker orders after they match.
Eventually this was done on purpose. Idea is that execution is guaranteed for an order once accepted by DEX module.
So that is why we skip validations for maker order.
Contributor
There was a problem hiding this comment.
maybe we should document this better in code
But this behaviour is expected IMO
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.
Closes: https://app.clickup.com/t/868jpxw8u
Summary
Immunefi 77114 reported that DEX settlement bypassed asset/ft compliance hooks on the maker side: only the taker (
order.Creator) was validated at match time, so freeze / whitelist /block_smart_contractschanges applied to a maker after order placement were silently ignored, letting frozen tokens drain out of the maker's account.This PR re-runs
validateCoinSpendableon the sender andvalidateCoinReceivableon the recipient of every settlement transfer inDEXExecuteActions, closing all three sibling exploits with one change. Regression test added inx/dex/keeper/keeper_ft_test.go.Context
Integration test changes
TestLimitOrdersMatchingWithAssetFTFreezeandTestLimitOrdersMatchingWithAssetFTWhitelistwere written assuming the buggy behavior was correct - with a comment that said "Reducing the whitelist limitwill not interfere with DEX order after order placement".
But this behavior is incorrect as reported in 77114: a maker places an order, the issuer freezes
or de-whitelists the account, and a taker can still drain the locked tokens.
With this fix the maker's freeze / whitelist state at match time is now respected, so those tests are updated to expect the match to be rejected and balances to stay intact.
Reviewers checklist:
Authors checklist
This change is