Skip to content

fix(asset/ft): enforce maker compliance at DEX settlement#138

Open
metalarm10 wants to merge 5 commits into
masterfrom
john/dex-settlement-maker-compliance-77114
Open

fix(asset/ft): enforce maker compliance at DEX settlement#138
metalarm10 wants to merge 5 commits into
masterfrom
john/dex-settlement-maker-compliance-77114

Conversation

@metalarm10

@metalarm10 metalarm10 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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_contracts changes applied to a maker after order placement were silently ignored, letting frozen tokens drain out of the maker's account.

This PR re-runs validateCoinSpendable on the sender and validateCoinReceivable on the recipient of every settlement transfer in DEXExecuteActions, closing all three sibling exploits with one change. Regression test added in x/dex/keeper/keeper_ft_test.go.

Context

Integration test changes

TestLimitOrdersMatchingWithAssetFTFreeze and TestLimitOrdersMatchingWithAssetFTWhitelist were written assuming the buggy behavior was correct - with a comment that said "Reducing the whitelist limit
will 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:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@metalarm10 metalarm10 changed the title <DO NOT MERGE - CI check> fix(asset/ft): validate maker compliance at DEX settlement (Immunefi … fix(asset/ft): enforce maker compliance at DEX settlement Jun 9, 2026
@metalarm10 metalarm10 marked this pull request as ready for review June 9, 2026 07:07
@metalarm10 metalarm10 requested a review from a team as a code owner June 9, 2026 07:07
@metalarm10 metalarm10 requested review from bashash, grainerycafe and ysv and removed request for a team June 9, 2026 07:07
"to", send.ToAddress.String(),
"coin", send.Coin.String(),
)
if err := k.validateDEXSettlementSend(ctx, send); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should document this better in code
But this behaviour is expected IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants