joicygiore - The user has no incentive to call `credits.aleo::split`, the expected `10_000u64` fee in that method will never be charged

[sherlock-admin4]

joicygiore

Medium

The user has no incentive to call credits.aleo::split, the expected 10_000u64 fee in that method will never be charged

Summary

The user has no incentive to call credits.aleo::split, the expected 10_000u64 fee in that method will never be charged

Vulnerability Detail

The credits.aleo::split method is used to split credits.record and output the split credits.record after charging a fee of 10_000u64, but users can use free methods to split credits.record

function split:
    // Input the record.
@>    input r0 as credits.record;
    // Input the amount to split.
    input r1 as u64.private;
    // Checks the given record has a sufficient amount to split.
    // This `sub` operation is safe, and the proof will fail
    // if an underflow occurs.
    sub r0.microcredits r1 into r2;
    // Checks the given record has a sufficient fee to remove.
    // This `sub` operation is safe, and the proof will fail
    // if an underflow occurs.
@>    sub r2 10_000u64 into r3;
    // Construct the first record.
    cast r0.owner r1 into r4 as credits.record;
    // Construct the second record.
    cast r0.owner r3 into r5 as credits.record;
    // Output the first record.
@>    output r4 as credits.record;
    // Output the second record.
@>    output r5 as credits.record;

The credits.aleo::transfer_private method is as follows. This method is almost the same as credits.aleo::split, and the user experience is even better than credits.aleo::split. As a result, users use credits.aleo::transfer_private to split credits.record, and the project owner will never receive the 10_000u64 fee in credits.aleo::split.

function transfer_private:
    // Input the sender's record.
@>    input r0 as credits.record;
    // Input the receiver.
    input r1 as address.private;
    // Input the amount.
    input r2 as u64.private;
    // Checks the given record has a sufficient amount.
    // This `sub` operation is safe, and the proof will fail
    // if an underflow occurs. The destination register `r3` holds
    // the change amount to be returned to the sender.
    sub r0.microcredits r2 into r3;
    // Construct a record for the specified receiver.
    cast r1 r2 into r4 as credits.record;
    // Construct a record with the change amount for the sender.
    cast r0.owner r3 into r5 as credits.record;
    // Output the receiver's record.
@>    output r4 as credits.record;
    // Output the sender's change record.
@>    output r5 as credits.record;

poc

please add to synthesizer/process/src/tests/test_credits.rs -> mod sanity_checks {} and run

    #[test]
    fn test_transfer_private_instead_of_split() {

        let rng = &mut TestRng::default();

        // Initialize a new caller account.
        let private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
        let caller = Address::try_from(&private_key).unwrap();
        // Initialize a new receiver account.
        let receiver_private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
        let receiver = Address::try_from(&receiver_private_key).unwrap();
        // Construct a new process. 
        let process = Process::load().unwrap();
        // Retrieve the stack.
        let stack = process.get_stack(ProgramID::from_str("credits.aleo").unwrap()).unwrap();

        // Declare the function name.
        let function_name = Identifier::from_str("transfer_private").unwrap();

        // Declare the inputs.

        let r0 = Value::from_str(&format!(
            "{{ owner: {caller}.private, microcredits: 1_500_000_000_000_000_u64.private, _nonce: {}.public }}",
            console::types::Group::<CurrentNetwork>::zero()
        ))
        .unwrap();

        // first caller -> caller
        // r1 caller
        println!("The caller splits its own `credits.record`");
        println!("caller address:{}",caller);
        let r1 = Value::<CurrentNetwork>::from_str(&format!("{caller}")).unwrap();
        let r2 = Value::<CurrentNetwork>::from_str("500_000_000_000_000_u64").unwrap();
        let assignment = get_assignment::<_, CurrentAleo>(stack, &private_key, function_name, &[r0.clone(), r1, r2], rng);

        // second caller -> receiver
        // r3 receiver
        println!("The caller splits its own `credits.record` and gives it to the receiver");
        println!("receiver address:{}",receiver);
        let r3 = Value::<CurrentNetwork>::from_str(&format!("{receiver}")).unwrap();
        let r4 = Value::<CurrentNetwork>::from_str("500_000_000_000_000_u64").unwrap();
        let assignment = get_assignment::<_, CurrentAleo>(stack, &private_key, function_name, &[r0.clone(), r3, r4], rng);
    }

output:

running 1 test
test tests::test_credits::sanity_checks::test_transfer_private_instead_of_split ... 
Initializing 'TestRng' with seed '12919731577808958542'
# first
The caller splits its own `credits.record`
caller address:aleo1v8yly0wddnhw8f8g0mm27way5e477qaczy2xel5amd0c7a40fsgqgcnfqd

response: Response { output_ids: [Record(1946212592058263978476207649632073510509913197475145696189401042673517860011field, 2082851168713763655056146131357467491414610678988027130276964193375923755171field), Record(4813665962502436734939872732516132741440382594417491589141052084820430580field, 2307006226474590043159930787178018015229668727600650851457650942944324635136field)], outputs: [{
  owner: aleo1v8yly0wddnhw8f8g0mm27way5e477qaczy2xel5amd0c7a40fsgqgcnfqd.private,
  microcredits: 500000000000000u64.private,
  _nonce: 7943999556933030435055786619308667226067777781080731267898175808825530343820group.public
}, {
  owner: aleo1v8yly0wddnhw8f8g0mm27way5e477qaczy2xel5amd0c7a40fsgqgcnfqd.private,
  microcredits: 1000000000000000u64.private,
  _nonce: 7760104168515769562844442175138568610741601504335729000581898828123044935267group.public
}] }
# second
The caller splits its own `credits.record` and gives it to the receiver
receiver address:aleo1w6pm2l53jyl7xff0zdg8hwmrr360qqx6pj7pfxdjjfydjq4zycyq85yptu
response: Response { output_ids: [Record(1304428865989802346632025960700613385762629084861123638886438637905370343412field, 250149739679050994235853485915200602362345388137455057686523068098998907775field), Record(3429420798034451281273456986880586221341871314168614472901402904135499680789field, 1085610665383722389481408806364663535393857944196118698576787754390980576298field)], outputs: [{
  owner: aleo1w6pm2l53jyl7xff0zdg8hwmrr360qqx6pj7pfxdjjfydjq4zycyq85yptu.private,
  microcredits: 500000000000000u64.private,
  _nonce: 3086607573272060536188420518033861532601668494684445818957031234700664674142group.public
}, {
  owner: aleo1v8yly0wddnhw8f8g0mm27way5e477qaczy2xel5amd0c7a40fsgqgcnfqd.private,
  microcredits: 1000000000000000u64.private,
  _nonce: 6980174604388008099452111185164303108185994192762640382187766847442151193579group.public
}] }
ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 78 filtered out; finished in 1.76s

Impact

The user has no incentive to call credits.aleo::split, the expected 10_000u64 fee in that method will never be

Code Snippet

https://github.com/ProvableHQ/snarkVM/blob/a3c9403a581ab03be3fdd075860e815c72e35065/synthesizer/program/src/resources/credits.aleo#L947-L972 https://github.com/ProvableHQ/snarkVM/blob/a3c9403a581ab03be3fdd075860e815c72e35065/synthesizer/program/src/resources/credits.aleo#L829-L850

Tool used

Manual Review

Recommendation

If you are sure that you need to charge a split fee, we recommend adding the corresponding charging logic in the credits.aleo::transfer_private method.

[evanmarshall]

transfer_private requires a fee at the consensus layer & split doesn't require a fee at the consensus layer (split is the only function that does not, every other function on every other program requires a fee). transfer_private does not require a fee natively in the credits.aleo program and split charges a fee natively in the credits.aleo program.

The reason for this is to handle an edge case where a user has only a single credits record but cannot transfer it because they have no public balance (fee_public) and no other records (fee_private). So the user can call split with a single record without a fee transition and the native fee prevents DOSing

[joicygiore]

transfer_private requires a fee at the consensus layer & split doesn't require a fee at the consensus layer (split is the only function that does not, every other function on every other program requires a fee). transfer_private does not require a fee natively in the credits.aleo program and split charges a fee natively in the credits.aleo program.

The reason for this is to handle an edge case where a user has only a single credits record but cannot transfer it because they have no public balance (fee_public) and no other records (fee_private). So the user can call split with a single record without a fee transition and the native fee prevents DOSing

@evanmarshall Hello sir, please excuse my interruption.

I am confused. If the following quote is true, then the fee recipient address information should be updated in credits.aleo::split, because the call occurs in credits.aleo, otherwise the fee will be lost.

transfer_private does not require a fee natively in the credits.aleo program and split charges a fee natively in the credits.aleo program.

The more confusing question is, in the face of this edge case, even if the user can call split, what is the meaning of the split? He just has one more credit, but still no fee_private or fee_public

The reason for this is to handle an edge case where a user has only a single credits record but cannot transfer it because they have no public balance (fee_public) and no other records (fee_private). So the user can call split with a single record without a fee transition and the native fee prevents DOSing

[evanmarshall]

By default in Aleo, base fees are burned for all transactions ie the supply decreases by the base fee amount.

The split function is intended to provide users a way who only have a single record (encrypted UTXO) and no public balance to still conduct a transaction. Without this setup for split, users who found themselves with a single record and no public balance would never be able to spend it.

[joicygiore]

By default in Aleo, base fees are burned for all transactions ie the supply decreases by the base fee amount.

The split function is intended to provide users a way who only have a single record (encrypted UTXO) and no public balance to still conduct a transaction. Without this setup for split, users who found themselves with a single record and no public balance would never be able to spend it.

But here is simply deducting costs from user credit. If the cost is calculated in a combustion, then we seem to update the total amount of money at the same time. I hope I didn't ignore anything, thank you

[sherlock-admin3]

Escalate

Quoting the sponsor's comments:

The reason for this is to handle an edge case where a user has only a single credits record but cannot transfer it because they have no public balance (fee_public) and no other records (fee_private). So the user can call split with a single record without a fee transition and the native fee prevents DOSing

In a specific edge case, a user without fee_public and fee_private, the only method they can call is credits.aleo::split. But no matter how they split, they will keep record.owner == self.signer. The only difference is that the user gets 2 or more credits.record.

Quoting the sponsor's comments again:

The split function is intended to provide users a way who only have a single record (encrypted UTXO) and no public balance to still conduct a transaction. Without this setup for split, users who found themselves with a single record and no public balance would never be able to spend it.

This operation still does not solve the problem of being unable to trade. In the end, a certain amount of funds are still needed to pay the consensus layer fee (however, there are many ways to obtain relevant funds, and it is not as difficult as imagined). Combined with the quote below, users still have no motivation to execute credits.aleo::split, and ultimately the 10_000u64 fee will never be charged

A fee is already charged (at the program layer instead of consensus layer). The base fee is about twice was a transfer_private requires as a base fee.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

It is clear from the sponsor's comments that this is a design decision. Also, a user can use transfer_private() for the same purpose. Just because a user has no motivation to call a function, I don't think we can classify it as Medium severity. For me, this problem is at most Low severity.

[joicygiore]

It is clear from the sponsor's comments that this is a design decision. Also, a user can use transfer_private() for the same purpose. Just because a user has no motivation to call a function, I don't think we can classify it as Medium severity. For me, this problem is at most Low severity.

Sir, I accept your judgment. But it is not because the credits.aleo::split-related issues involved here are not enough to be medium, but because my original report did not mention the relevant content. Thank you for your patience in replying.

[infosecual]

But here is simply deducting costs from user credit. If the cost is calculated in a combustion, then we seem to update the total amount of money at the same time. I hope I didn't ignore anything, thank you

It is worth noting that all fees are burned in Aleo whether they are consumed in-program (like in the split case), or in fee commitments like al other cases. This is affecting the overall money supply but this is known and by design.

[joicygiore]

But here is simply deducting costs from user credit. If the cost is calculated in a combustion, then we seem to update the total amount of money at the same time. I hope I didn't ignore anything, thank you

It is worth noting that all fees are burned in Aleo whether they are consumed in-program (like in the split case), or in fee commitments like al other cases. This is affecting the overall money supply but this is known and by design.

I'm not sure if I should agree with this statement. If so, I will raise another question. The report #31mentioned that some problems will occur when the validator and the delegator use the same withdrawal address. In other words, should this be the user's own problem? Consider a scenario where credits.aleo::split and credits.aleo::transfer_private can achieve the same effect, but the fees are different. Then user A uses credits.aleo::split to split and user B uses credits.aleo::transfer_private to split, then A pays twice the cost of B. Should this be counted as a loss for user A? Because A doesn't know, right?

Finally, I accept all the decisions of the judge.

[infosecual]

I am not following what you mean by the last message.

I think I understand where the confusion is on this issue though- the title of this submission is "The user has no incentive to call credits.aleo::split". This is incorrect and I will attempt to explain why-

Some background - What are the relevant differences between split and transfer_private?

split:

1. fee is charges directly in the program
2. the caller only needs a record, no pubic balance is needed to call split

transfer_private-

1. the fee is charged via a fee commitment
2. the caller needs a record AND a public balance

So why would a user ever use split over transfer_private? They will do this if they do not have a public balance.

This is inline with the sponsor's comment:

The split function is intended to provide users a way who only have a single record (encrypted UTXO) and no public balance to still conduct a transaction. Without this setup for split, users who found themselves with a single record and no public balance would never be able to spend it.

This is why a user would call split. This is why the claim that "The user has no incentive to call credits.aleo::split" is incorrect. The user will have no choice to use anything but split if they do not have a public balance. split is provided specifically for this reason.

[joicygiore]

I am not following what you mean by the last message.

I think I understand where the confusion is on this issue though- the title of this submission is "The user has no incentive to call credits.aleo::split". This is incorrect and I will attempt to explain why-

Some background - What are the relevant differences between split and transfer_private?

split:

1. fee is charges directly in the program
2. the caller only needs a record, no pubic balance is needed to call split

transfer_private-

1. the fee is charged via a fee commitment
2. the caller needs a record AND a public balance

So why would a user ever use split over transfer_private? They will do this if they do not have a balance.

This is inline with the sponsor's comment:

The split function is intended to provide users a way who only have a single record (encrypted UTXO) and no public balance to still conduct a transaction. Without this setup for split, users who found themselves with a single record and no public balance would never be able to spend it.

This is why a user would use split. This is why the claim that "The user has no incentive to call credits.aleo::split" is incorrect. The user will have no choice to use anything but split if they do not have a public balance.

I don't really want to argue about this. But even if the user has a public balance, he can still use splitting and will not be blocked, right? The result is that he pays more cost for the call. As for why he called it, I don't quite understand what he thinks. It is probably the same as setting the same withdrawal address. It depends on whether he is happy. In fact, the key point of this issue is that I haven't figured out whether a user without a public balance calls this method and whether he can get the public balance required to call other methods, and how splitting allows him to get the public balance. I think this is more meaningful to me than whether this question is valid.

[joicygiore]

Sir, I really hope you can tell me the process of users calling credits.aleo::split to get the public balance, thank you. Although I don't know his process at present, but if you are willing to teach me, I believe I can learn it.

[infosecual]

But even if the user has a public balance, he can still use splitting and will not be blocked, right?

Yes. The issue is not that he is blocked from using split if he has a public balance. It is that he cannot use transfer_private if he does not have a public balance. Here is something to illustrate this:

User has a public balance?	can use split	can use transfer_private	User has the following options:
yes	yes	yes	can use either transfer_private or split
no	yes	no	must use split

When a user does not have a public balance they must use split because they cannot use transfer_private. split was created specifically for users in this situation in the last row.

[joicygiore]

So, do users lose funds by using split? Do users get public balances by using split?

[infosecual]

They do not lose funds. They pay the split fee of 10 credits (which is burned). They put one record in and get 2 records out. They do not put any public balance in and do not receive any public balance out.

[joicygiore]

Is the cost greater than transfer_private? A user splits 100 times to get 101 credits, spending 10_000u64 * 100, and he still cannot call other methods. What's the point?

[WangSecurity]

Firstly, I cannot see the escalated message, so I assume it was deleted?

Secondly, I don't see any issue in the code and it works perfectly as intended.

Thirdly, this report is only about one function having larger fees than others, hence, lower incentives. I believe it's not an issue and it's up to the protocol to determine the fees. Each function has its purpose which has been perfectly explained by the Sponsor and LSW.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• joicygiore: rejected