Always require funder auth - #21
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens authorization semantics across the channel and channel-factory Soroban contracts by requiring explicit funder authorization even when the deposit/top-up amount is zero, and adds tests/snapshots to enforce this behavior.
Changes:
- Require funder authorization for zero-amount
top_upand initial deposit during__constructor. - Require funder authorization for zero-amount channel deployments via the factory
open. - Add/refresh tests and snapshot fixtures to assert the expected auth invocation trees for zero-amount operations.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates user-facing docs to reflect that channel opening is deployer-initiated but funder-authorized (no zero-amount auth bypass). |
| contracts/channel/src/lib.rs | Makes top_up require funder auth regardless of amount; constructor docs updated accordingly. |
| contracts/channel/src/test.rs | Adds/updates tests asserting auth is required for zero-amount top_up and zero-amount channel construction. |
| contracts/channel/test_snapshots/test/test_top_up_zero.1.json | Updates snapshot to include funder auth + transfer(0) sub-invocation. |
| contracts/channel/test_snapshots/test/test_open_zero_amount.1.json | New snapshot covering channel construction with zero amount requiring auth. |
| contracts/channel-factory/src/lib.rs | Makes factory open always require funder auth (including for amount 0) and updates docs. |
| contracts/channel-factory/src/test.rs | Adds test asserting auth tree for factory open with zero initial deposit. |
| contracts/channel-factory/test_snapshots/test/test_open.1.json | Updates snapshot due to auth/wasm/code changes. |
| contracts/channel-factory/test_snapshots/test/test_open_zero_amount.1.json | New snapshot covering factory open with zero amount requiring auth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn top_up(env: &Env, amount: i128) { | ||
| assert_with_error!(env, amount >= 0, Error::NegativeAmount); | ||
| if amount > 0 { | ||
| // Transfer tokens from the funder to the channel. | ||
| let from = Self::from(env); | ||
| from.require_auth(); | ||
| Self::token_client(env).transfer(&from, &env.current_contract_address(), &amount); | ||
| } | ||
|
|
||
| // Transfer tokens from the funder to the channel. | ||
| let from = Self::from(env); | ||
| from.require_auth(); | ||
| Self::token_client(env).transfer(&from, &env.current_contract_address(), &amount); |
leighmcculloch
changed the title
f985205 to
fa474e4
Compare
| let from = Self::from(env); | ||
| from.require_auth(); | ||
| if amount > 0 { | ||
| // Transfer tokens from the funder to the channel. | ||
| let from = Self::from(env); | ||
| from.require_auth(); |
There was a problem hiding this comment.
I think the change here could go a couple different ways:
- It could do as is currently done here, always require auth, and only transfer on > 0.
- It could always transfer too.
- It could do nothing new, keep the behaviour the same for top_up, and the __constructor could get a from.require_auth() always instead, and in this way top_up would not require auth when 0, although that seems like an odd case to cater for.
There was a problem hiding this comment.
- It could always transfer too.
Nah, an extra no-op cross-contract call is expensive and undesirable here.
There was a problem hiding this comment.
Is the top_up function accepting amount == 0 here just so the constructor can call it with zero initial balance?
There was a problem hiding this comment.
I guess so, I considered what if we changed it and the constructor did the initial conditioned topup itself and topup always did a transfer, but given that changes behaviour in a larger way wasn't clear we should do that at this stage.
leighmcculloch
changed the title
marcelosalloum
left a comment
marcelosalloum
left a comment
There was a problem hiding this comment.
LGTM! A squatter can no longer create a from-bound channel at the deterministic address without from's signature, even at amount == 0.
| let from = Self::from(env); | ||
| from.require_auth(); | ||
| if amount > 0 { | ||
| // Transfer tokens from the funder to the channel. | ||
| let from = Self::from(env); | ||
| from.require_auth(); |
There was a problem hiding this comment.
- It could always transfer too.
Nah, an extra no-op cross-contract call is expensive and undesirable here.
| let from = Self::from(env); | ||
| from.require_auth(); | ||
| if amount > 0 { | ||
| // Transfer tokens from the funder to the channel. | ||
| let from = Self::from(env); | ||
| from.require_auth(); |
There was a problem hiding this comment.
Is the top_up function accepting amount == 0 here just so the constructor can call it with zero initial balance?
92a6c04 to
bd88100
Compare
bd88100 to
4e9e9c3
Compare
Warning
Intended to merge to main after #22.
What
Drop the
amount > 0branches so that opening a channel and topping one up always require the funder's authorization.Why
Keep the channel contracts consistent on when auth is required, so they require auth on the same functions every time even in the zero case. Originally this was left off on the idea that someone else might create the channel first, but this is not a realistic expectation because the commitment key must be known at channel open time. So either way the funder has to be involved and there must be coordination.
The SAC's own
transferauthorizes and transfers unconditionally at zero amounts, so matching it removes branches and the assumptions those branches break when the channel is composed with other contracts. In the factory the funder has to sign regardless, since they supply their own commitment key.