fix(security): enforce escrow mediation in repayment flow - #116
fix(security): enforce escrow mediation in repayment flow#116Joseph-1-Duro wants to merge 3 commits into
Conversation
|
@Joseph-1-Duro Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
fix Ci error @Joseph-1-Duro |
03e53e8 to
8aa9600
Compare
|
Please Resolve Merge conflict @Joseph-1-Duro |
Resolved conflicts: - escrow/src/lib.rs: use > check (prevent overpayment) - escrow/src/test.rs: rename test to test_release_to_pool_fails_on_overpayment, use amount + 1 - invoice/src/lib.rs: combine amount param with escrow routing - Escrow/pool snapshots: keep HEAD (escrow-routed) versions
0e49c54 to
acedc4a
Compare
|
Please resolcve merge conflict @Joseph-1-Duro |
|
No CI checks have run on this PR. Please rebase onto |
K1NGD4VID
left a comment
K1NGD4VID
left a comment
There was a problem hiding this comment.
Review Feedback
This PR adds a caller parameter to release_to_pool and validates the caller is either the pool or invoice contract. This is a breaking API change to the escrow contract's public interface.
Concerns:
-
Very stale — This PR was opened months ago. The escrow and invoice contracts have evolved significantly since then. The current codebase likely has different authorization patterns that make this change incompatible.
-
Breaking change — Adding a parameter to
release_to_poolrequires updating every call site (invoice contract, pool contract, tests). The current escrow contract onmainalready has properpool.require_auth()on this function. -
No CI — CI hasn't run, and given the staleness, it's very likely to fail.
Recommendation: Please rebase onto main and check whether the security concern this PR addresses is still relevant. The current main already has pool.require_auth() in release_to_pool, which may already solve the issue. If the current auth model is sufficient, this PR can be closed.
2 participants
close #59