Skip to content

Loan manager deposit collateral cei fix#27

Merged
ogazboiz merged 7 commits into
LabsCrypt:mainfrom
jotel-dev:loan-manager-deposit-collateral-cei-fix
Jun 20, 2026
Merged

Loan manager deposit collateral cei fix#27
ogazboiz merged 7 commits into
LabsCrypt:mainfrom
jotel-dev:loan-manager-deposit-collateral-cei-fix

Conversation

@jotel-dev

@jotel-dev jotel-dev commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

I added a regression test that installs a malicious token contract which removes the loan during transfer
verifies deposit_collateral returns Err(LoanError::LoanNotFound) safely
Next:

closes #17

@ogazboiz ogazboiz left a comment

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.

nice work on this one. the core restructuring is exactly right: you commit the updated collateral to storage before the token transfer and replaced every expect with a typed error, so it now matches the cei pattern used in repay and approve_loan. the malicious-token regression test is a great touch and the assertion (Err(LoanError::LoanNotFound)) lines up with the re-read path.

that said, the pr currently does not compile, so i cannot merge it yet. a few asks:

  1. loan_manager/src/lib.rs:1356 - let loan: Loan needs to be let mut loan: Loan. line 1390 mutates loan.collateral_amount, so the build fails with E0594 (cannot assign, not declared as mutable).

  2. multisig_governance/src/test.rs:5-6 - the //! inner doc comment sits after a use statement, which rust rejects (E0753). switch those two lines to regular // comments.

  3. run cargo fmt - the new import line in loan_manager/src/test.rs:3 needs to be split across lines to pass cargo fmt --check.

  4. the multisig_governance changes (count_valid_approvals plus its test) look like they came in from a different branch. could you rebase onto main so this pr only contains the deposit_collateral fix? that keeps it scoped to issue #17 and is what introduces the E0753 above.

one small optional note: the event at lib.rs:1407 still publishes the pre-transfer loan fields rather than the re-read stored_loan. it is harmless since state is already durable before the transfer, just flagging it.

once it compiles and cargo fmt/clippy/test are green, this should be good to go. the logic itself is solid.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

@jotel-dev

Copy link
Copy Markdown
Contributor Author

okay coming

@jotel-dev jotel-dev requested a review from ogazboiz June 20, 2026 10:02
@jotel-dev

Copy link
Copy Markdown
Contributor Author

done @ogazboiz

@ogazboiz ogazboiz left a comment

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.

all four blockers addressed: the let mut loan compiles, the unrelated multisig_governance changes are dropped so it's correctly scoped to deposit_collateral, cargo fmt is clean, and the cei is right, state is committed before the transfer with every expect replaced by typed errors and a re-read that rejects a loan removed mid-transfer (LoanNotActive/LoanNotFound). the MaliciousToken regression test confirms it. ci is green. merging.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

@ogazboiz ogazboiz merged commit 4423ab6 into LabsCrypt:main Jun 20, 2026
1 check passed
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.

[Contracts] LoanManager deposit_collateral re-reads the loan after a token transfer, creating a check-then-act gap

2 participants