Skip to content

fix(collateralize,issuance): return repaid collateral to borrow record owner - #1308

Open
33cn wants to merge 2 commits into
masterfrom
fix-repay-collateral-owner
Open

fix(collateralize,issuance): return repaid collateral to borrow record owner#1308
33cn wants to merge 2 commits into
masterfrom
fix-repay-collateral-owner

Conversation

@33cn

@33cn 33cn commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Vulnerability

Third-party repayment steals the borrower's over-collateralized BTY.

  • plugin/dapp/collateralize/executor/collateralizedb.go:663-769 (CollateralizeRepay): at line 742 the collateral is returned via ExecTransferFrozen(coll.CreateAddr, action.fromaddr, ...) — to the transaction sender, not to borrowRecord.AccountAddr. There is no borrower identity check anywhere in the function.
  • plugin/dapp/issuance/executor/issuancedb.go:630-719 (IssuanceRepay): same flaw at line 691 — collateral goes to action.fromaddr instead of debtRecord.AccountAddr.

Root cause

Any account can repay an arbitrary borrow/debt record (this itself is a legitimate repay-on-behalf feature), but the collateral refund target was hardcoded to the caller. An attacker therefore pays only the debt principal (plus the small stability fee in collateralize) and receives the borrower's full collateral — about 4x the repayment cost at a 0.25 liquidation ratio.

Fix

Change the collateral refund target, not the repayment permission:

  • After the fork, the collateral is returned to the borrow/debt record owner (record.AccountAddr); the caller only loses the repayment funds.
  • Third-party repayment remains allowed. Neighboring functions show the design is not borrower-exclusive: CollateralizeAppend lets any address add collateral to someone else's record, and neither repay path ever checked fromaddr == record.AccountAddr. So repay-on-behalf is kept as a feature and only the theft vector (wrong refund target) is closed.

Fork gating

New dapp forks registered at height 0, following the ForkEVMFixOverflow pattern:

  • ForkCollateralizeRepayOwner in plugin/dapp/collateralize/types (RegisterDappFork in InitFork, gated via cfg.IsDappFork in CollateralizeRepay)
  • ForkIssuanceRepayOwner in plugin/dapp/issuance/types (same pattern in IssuanceRepay)

Both are also added to chain33.fork.toml alongside the existing sibling forks. Before the fork the old behavior (refund to caller) is preserved so running chains keep consensus.

Tests

New regression tests (no changes to existing tests):

  • plugin/dapp/collateralize/executor/vuln_fix_test.go
    • TestRepayOwnerForkRepayByNonBorrower: post-fork, a third-party repayer's collateral stays untouched; collateral goes back to the borrower's exec account; repayer only loses debt+fee, which is less than the collateral value.
    • TestRepayOwnerForkRepayByBorrower: borrower self-repay still works and returns the collateral.
    • TestRepayOwnerPreForkRepayByNonBorrower: pre-fork behavior unchanged (collateral to caller).
  • plugin/dapp/issuance/executor/vuln_fix_test.go: same three scenarios (TestRepayOwnerForkRepayByNonDebtor, TestRepayOwnerForkRepayByDebtor, TestRepayOwnerPreForkRepayByNonDebtor).

go test -ldflags=-checklinkname=0 ./plugin/dapp/collateralize/... ./plugin/dapp/issuance/... passes, including all pre-existing tests.

king added 2 commits August 31, 2026 10:51
…d owner

CollateralizeRepay and IssuanceRepay transferred the collateral back to
the transaction sender (action.fromaddr) instead of the borrow/debt
record owner. Any third party could repay someone else's debt at the
cost of principal plus stability fee and steal the borrower's
over-collateralized BTY.

Gate the fix behind the new dapp forks ForkCollateralizeRepayOwner and
ForkIssuanceRepayOwner: after the fork the collateral is returned to
record.AccountAddr; before the fork the old behavior is preserved to
keep consensus of running chains. Third-party repayment itself remains
allowed (repay-on-behalf is a legitimate feature), only the collateral
refund target is fixed.
…d proxyminer configs

chain33 validates that every registered dapp fork has a config entry on
non-local titles; missing entries crash the node at startup, which broke
ci_cross2eth/ci_parachain_rollup/ci_paracross/ci_rgbx.
@vipwzw
vipwzw requested a review from bysomeone September 8, 2026 02:47

@bysomeone bysomeone left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

关于底层缺陷与本次修复

根因在于 CollateralizeRepay/IssuanceRepay 从未把"谁在还款"和"抵押物退给谁"绑定到借据 owner:任何地址都能替任意借据还款(无 fromaddr == record.AccountAddr 校验),并把冻结抵押物退给 action.fromaddr(发起人)而非 record.AccountAddr(借据 owner)。

产品层面看,这只可能是两种情况之一:要么 Repay 当初实现时漏了 owner 校验;要么有意支持第三方代还(让借款人可请他人代还自救),但抵押物归还没有配套绑回借据 owner——于是留下套利窗口:代还者仅以约 1/4 的成本拿走全额抵押物。

本次修复是共识层缺陷下正确的最小改动:保留代还权限不变,只把抵押物退回目标改为借据 owner,并挂在新增的 ForkCollateralizeRepayOwner/ForkIssuanceRepayOwner fork 之后,保证 fork 前旧链行为逐字节一致。核心修复、fork 接线、回归测试均验证通过。

一个遗留的小不一致(非阻塞,仅本地查询层,不影响 state root/共识)

fork 后抵押物实际退回 record.AccountAddr,但 Repay 的 receipt log 仍记录 AccountAddr = action.fromaddr(代还发起人);exec_local 又用该字段作为 record 表的地址二级索引(record 主键是 RecordId)。于是第三方代还后,同一条 RecordId 的地址索引从 borrower 移到了 repayer(已实测:post-fork 下 C 代还 B → B 名下 0 条、C 名下 1 条 status=Close)。后果:borrower 通过按地址查询(Query_CollateralizeRecordByAddr 等)会看不到这笔已还记录,记录反而出现在代还者名下。建议 fork 生效后把 log 里的归属记为借据 owner,或同时携带 owner 与 repayer,使本地索引与资金去向保持一致。

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