Skip to content

chore:verify recipient signature via snip12 #130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mubarak23 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore:verify recipient signature via snip12 #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/base/errors.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,8 @@ pub mod Errors {

// Throw Error when Fee percent exceeds 100%
pub const PROTOCOL_FEE_PERCENTAGE_EXCEED: felt252 = 'Error: Fee percent exceeds 100%';

// Throw Error when Invalid recipient signature
pub const INVALID_RECIPIENT_SIGNATURE: felt252 = 'Error: No recipient signature';
}

42 changes: 38 additions & 4 deletions src/payment_stream.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#[starknet::contract]
pub mod PaymentStream {
use core::ecdsa::check_ecdsa_signature;
use core::hash::{HashStateExTrait, HashStateTrait};
use core::num::traits::{Bounded, Zero};
use core::pedersen::PedersenTrait;
use core::traits::Into;
use fundable::interfaces::IPaymentStream::IPaymentStream;
use openzeppelin::access::accesscontrol::AccessControlComponent;
Expand All @@ -13,6 +16,10 @@ pub mod PaymentStream {
use openzeppelin::token::erc721::{ERC721Component, ERC721HooksEmptyImpl};
use openzeppelin::upgrades::UpgradeableComponent;
use openzeppelin::upgrades::interface::IUpgradeable;
// use openzeppelin::utils::cryptography::interface::ISRC6;
use openzeppelin::utils::cryptography::snip12::{
OffchainMessageHash, SNIP12Metadata, StructHash,
};
Comment on lines +19 to +22
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unused imports and implement proper SNIP12 message hashing

The SNIP12 imports (OffchainMessageHash, SNIP12Metadata, StructHash) are imported but not used. The current get_message_hash implementation uses basic Pedersen hashing instead of SNIP12's structured message hashing with domain separation.

For proper SNIP12 compliance, either:

  1. Remove the unused imports if not implementing SNIP12
  2. Implement proper SNIP12 message hashing with domain separation
🤖 Prompt for AI Agents 
In src/payment_stream.cairo around lines 19 to 22, the SNIP12 imports are unused
and the get_message_hash function uses basic Pedersen hashing instead of SNIP12
structured hashing. To fix this, either remove the unused SNIP12 imports if you
do not plan to implement SNIP12 message hashing, or update get_message_hash to
use SNIP12's OffchainMessageHash or StructHash with domain separation to comply
with SNIP12 standards.

use starknet::storage::{
Map, MutableVecTrait, StoragePathEntry, StoragePointerReadAccess, StoragePointerWriteAccess,
Vec,
Expand All @@ -22,10 +29,11 @@ pub mod PaymentStream {
};
use crate::base::errors::Errors::{
DECIMALS_TOO_HIGH, FEE_TOO_HIGH, INSUFFICIENT_ALLOWANCE, INSUFFICIENT_AMOUNT,
INVALID_RECIPIENT, INVALID_TOKEN, NON_TRANSFERABLE_STREAM, ONLY_NFT_OWNER_CAN_DELEGATE,
OVERDEPOSIT, SAME_COLLECTOR_ADDRESS, SAME_OWNER, STREAM_CANCELED, STREAM_HAS_DELEGATE,
STREAM_NOT_ACTIVE, STREAM_NOT_PAUSED, TOO_SHORT_DURATION, UNEXISTING_STREAM,
WRONG_RECIPIENT, WRONG_RECIPIENT_OR_DELEGATE, WRONG_SENDER, ZERO_AMOUNT,
INVALID_RECIPIENT, INVALID_RECIPIENT_SIGNATURE, INVALID_TOKEN, NON_TRANSFERABLE_STREAM,
ONLY_NFT_OWNER_CAN_DELEGATE, OVERDEPOSIT, SAME_COLLECTOR_ADDRESS, SAME_OWNER,
STREAM_CANCELED, STREAM_HAS_DELEGATE, STREAM_NOT_ACTIVE, STREAM_NOT_PAUSED,
TOO_SHORT_DURATION, UNEXISTING_STREAM, WRONG_RECIPIENT, WRONG_RECIPIENT_OR_DELEGATE,
WRONG_SENDER, ZERO_AMOUNT,
};
use crate::base::types::{ProtocolMetrics, Stream, StreamMetrics, StreamStatus};

Expand Down Expand Up @@ -312,6 +320,30 @@ pub mod PaymentStream {

#[generate_trait]
impl InternalImpl of InternalTrait {
fn get_message_hash(self: @ContractState, stream_id: u256, approved: felt252) -> felt252 {
let mut hash_state = PedersenTrait::new(0);
hash_state = hash_state.update('CancelStreamMessage');
hash_state = hash_state.update(stream_id.low.into());
hash_state = hash_state.update(stream_id.high.into());
hash_state = hash_state.update(approved);
hash_state.finalize()
}
Comment on lines +323 to +330
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add replay protection and domain separation to message hash

The current message hash implementation has critical security issues:

  1. No replay protection: The same signature can be reused multiple times
  2. No domain separation: Signatures could be valid across different contracts
  3. Not SNIP12 compliant: Despite the PR title, this doesn't follow SNIP12 standard

Consider adding:

  • A nonce or timestamp for replay protection
  • Contract address for domain separation
  • Proper SNIP12 structured hashing
🤖 Prompt for AI Agents 
In src/payment_stream.cairo around lines 323 to 330, the get_message_hash
function lacks replay protection, domain separation, and SNIP12 compliance. To
fix this, include a nonce or timestamp parameter in the hash inputs to prevent
replay attacks, add the contract address to the hash to ensure domain
separation, and restructure the hashing logic to follow the SNIP12 standard
format for message hashing. Update the function signature and hash computation
accordingly to incorporate these elements.



fn mutual_cancel(
self: @ContractState,
stream_id: u256,
sig_r: felt252,
sig_s: felt252,
recipient_pubkey: felt252,
) -> bool {
let message_hash = self
.get_message_hash(stream_id, '1'); // 1 recipient has approve the cancellation
let is_valid = check_ecdsa_signature(message_hash, recipient_pubkey, sig_r, sig_s);
assert(is_valid, INVALID_RECIPIENT_SIGNATURE);
return true;
}
Comment on lines +323 to +345
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify test coverage for signature verification

The new signature verification functions lack test coverage. Please ensure comprehensive tests for:

  • Valid signature verification
  • Invalid signature rejection
  • Replay attack prevention
  • Edge cases (wrong stream_id, wrong recipient, etc.)

🏁 Script executed:

#!/bin/bash
# Search for tests related to mutual cancellation or signature verification
echo "Searching for signature verification tests..."
fd -e cairo -e rs test | xargs rg -l "mutual_cancel|recipient.*signature|snip12" | head -20

echo -e "\nSearching for any test files that might test the payment stream cancel functionality..."
fd -e cairo test | xargs rg -l "cancel.*stream|StreamCanceled" | head -10

Length of output: 532

Add comprehensive tests for mutual_cancel signature verification

I didn’t find any existing tests covering the mutual_cancel flow or ECDSA signature checks. Please add or extend tests in your suite to cover:

  • Valid signature acceptance for mutual_cancel
  • Rejection of invalid signatures (wrong r, s, or public key)
  • Replay-attack prevention (same signature cannot be reused)
  • Edge cases (incorrect stream_id, unauthorized recipient, zero/overflow values)

Suggested locations:

  • tests/test_payment_stream.cairo – extend with mutual_cancel scenarios
  • (Optional) a new file, e.g. tests/test_signature_verification.cairo, if you prefer to isolate signature tests
🤖 Prompt for AI Agents 
In src/payment_stream.cairo around lines 323 to 345, there are no tests covering
the mutual_cancel function and its ECDSA signature verification. Add
comprehensive tests in tests/test_payment_stream.cairo or a new test file like
tests/test_signature_verification.cairo to cover valid signature acceptance,
rejection of invalid signatures (altered r, s, or public key), replay attack
prevention by ensuring the same signature cannot be reused, and edge cases such
as incorrect stream_id, unauthorized recipient, and zero or overflow values.
These tests should call mutual_cancel with various inputs and assert expected
success or failure outcomes.

Comment on lines +333 to +345
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Fix signature verification logic and validate recipient

Several issues with the mutual_cancel implementation:

  1. Hardcoded approval value: Always passes '1' for approved, making the parameter meaningless
  2. Missing recipient validation: Should verify that recipient_pubkey matches the stream recipient's public key
  3. Redundant return type: Function returns bool but always returns true (reverts on failure)
  4. Not integrated: Function is defined but never called

🏁 Script executed:

#!/bin/bash
# Check if mutual_cancel is called anywhere in the codebase
echo "Searching for mutual_cancel function calls..."
rg "mutual_cancel" --type cairo -A 3 -B 3

Length of output: 172

Here's a refined check to locate any invocations of mutual_cancel in the repo:

🏁 Script executed:

#!/bin/bash
echo "Searching for calls to mutual_cancel..."
rg -n "mutual_cancel(" .

Length of output: 194

Let’s retry locating any calls (escaping the parenthesis):

🏁 Script executed:

#!/bin/bash
echo "Searching for calls to mutual_cancel..."
rg -n 'mutual_cancel\(' .

Length of output: 173

Revise and integrate mutual_cancel in the cancellation workflow

The new mutual_cancel at src/payment_stream.cairo:333–345 needs the following fixes before merging:

  • Ensure it’s actually invoked
    • No references found beyond its definition—either call it from your cancel function or remove it.
  • Remove or parameterize the hard-coded approval flag
    • Passing '1' into get_message_hash makes the “approved” argument meaningless. Use a dynamic flag or drop the parameter.
  • Validate the signer against the stored stream recipient
    • Lookup the recipient’s public key in state and assert that it matches recipient_pubkey before signature verification.
  • Simplify the return type
    • Since the function either returns true or reverts, change its signature to -> () (no return value) to avoid redundant booleans.
🤖 Prompt for AI Agents 
In src/payment_stream.cairo around lines 333 to 345, the mutual_cancel function
is defined but never called, so integrate it by invoking it from the main cancel
function or remove it if unused. Replace the hard-coded '1' approval flag in
get_message_hash with a dynamic parameter or remove it if unnecessary. Add a
check to retrieve the stored recipient public key from the contract state and
assert it matches the recipient_pubkey argument before verifying the signature.
Finally, change the function signature to return no value (-> ()) since it
either succeeds or reverts, eliminating the redundant boolean return.


fn assert_stream_exists(self: @ContractState, stream_id: u256) {
let stream = self.streams.read(stream_id);
assert(!stream.sender.is_zero(), UNEXISTING_STREAM);
Expand Down Expand Up @@ -970,6 +1002,8 @@ pub mod PaymentStream {
}

fn cancel(ref self: ContractState, stream_id: u256) {
// check for mutual cancel approval via SNIP12

Comment on lines 1004 to +1006
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incomplete implementation - signature verification not integrated

The comment indicates intent to check mutual cancellation approval, but the mutual_cancel function is never called. This means:

  1. The PR objective of "verify recipient signature via snip12" is not achieved
  2. Stream cancellation still works without recipient approval
  3. The new signature verification code is dead code

Either:

  • Complete the implementation by calling mutual_cancel with appropriate parameters
  • Remove the incomplete code until ready for full implementation

Would you like me to help implement the complete signature verification flow in the cancel function?

🤖 Prompt for AI Agents 
In src/payment_stream.cairo around lines 1004 to 1006, the cancel function
comments mention checking mutual cancel approval via SNIP12, but the
mutual_cancel function is never called, leaving signature verification
unimplemented and allowing cancellation without recipient approval. To fix this,
integrate a call to mutual_cancel within the cancel function, passing the
necessary parameters such as the stream_id and signature data, to enforce
recipient signature verification before allowing cancellation. Alternatively, if
the feature is not ready, remove the incomplete mutual_cancel related code to
avoid dead code and confusion.

self.reentrancy_guard.start();

// Ensure the caller is the stream sender
Expand Down