search

sherlock-audit / 2024-05-aleo-judging

0 stars 0 forks source link

joicygiore - Fee related methods only update the fee payer's account information, and do not update the recipient's account information. #18

Closed sherlock-admin2 closed 1 week ago

sherlock-admin2 commented 2 weeks ago

joicygiore

Medium

Fee related methods only update the fee payer's account information, and do not update the recipient's account information.

Summary

1.Each call to credits.aleo::split will lose 10_000u64 of microcredits 2.credits.aleo::fee_public and credits.aleo::fee_private are missing fee recipients, causing fees to be lost

Vulnerability Detail

credits.aleo::split

Looking at the credits.aleo::split function, we can see that the function splits input r0 as credits.record; into two new credits.record. However, the deducted 10_000u64 is not credited to any account or credits.record, which results in the loss of 10_000u64 every time the method is called

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;

credits.aleo::fee_public and credits.aleo::fee_private

credits.aleo::fee_public only updates the sender's account information, but not the recipient's information, resulting in fee loss, and does not check and record the relevant information of deployment or execution ID.

// The `fee_public` function charges the specified amount from the sender's account.
function fee_public:
    // Input the amount.
    input r0 as u64.public;
    // Input the priority fee amount.
    input r1 as u64.public;
    // Input the deployment or execution ID.
    input r2 as field.public;
    // Ensure the amount is nonzero.
    assert.neq r0 0u64;
    // Ensure the deployment or execution ID is nonzero.
    assert.neq r2 0field;
    // Add the fee and priority fee amounts.
    add r0 r1 into r3;
    // Decrement the balance of the sender publicly.
    async fee_public self.signer r3 into r4;
    // Output the finalize future.
    output r4 as credits.aleo/fee_public.future;

finalize fee_public:
    // Input the sender's address.
    input r0 as address.public;
    // Input the total fee amount.
    input r1 as u64.public;
    // Retrieve the balance of the sender.
    // If `account[r0]` does not exist, `fee_public` is reverted.
    get account[r0] into r2;
    // Decrements `account[r0]` by `r1`.
    // If `r2 - r1` underflows, `fee_public` is reverted.
    sub r2 r1 into r3;
    // Updates the balance of the sender.
@>    set r3 into account[r0];

The credits.aleo::fee_private method only returns the credits.record after deducting the fee, and does not process the account information of the fee recipient (or output the corresponding credits.record), resulting in the loss of fees, and does not check and record the relevant information of the deployment or execution ID.

// The `fee_private` function charges the specified amount from the sender's record.
function fee_private:
    // Input the sender's record.
    input r0 as credits.record;
    // Input the amount.
    input r1 as u64.public;
    // Input the priority fee amount.
    input r2 as u64.public;
    // Input the deployment or execution ID.
    input r3 as field.public;
    // Ensure the amount is nonzero.
    assert.neq r1 0u64;
    // Ensure the deployment or execution ID is nonzero.
    assert.neq r3 0field;
    // Add the fee and priority fee amounts.
    add r1 r2 into r4;
    // 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 for the sender.
    sub r0.microcredits r4 into r5;
    // Construct a record with the change amount for the sender.
    cast r0.owner r5 into r6 as credits.record;
    // Output the sender's change record.
@>    output r6 as credits.record;

Impact

1.Each call to credits.aleo::split will lose 10_000u64 of microcredits 2.credits.aleo::fee_public and credits.aleo::fee_private are missing fee recipients, causing fees to be lost

Code Snippet

https://github.com/sherlock-audit/2024-05-aleo/blob/55b2e4a02f27602a54c11f964f6f610fee6f4ab8/snarkVM/synthesizer/program/src/resources/credits.aleo#L947-L972 https://github.com/sherlock-audit/2024-05-aleo/blob/55b2e4a02f27602a54c11f964f6f610fee6f4ab8/snarkVM/synthesizer/program/src/resources/credits.aleo#L1005-L1035 https://github.com/ProvableHQ/snarkVM/blob/a3c9403a581ab03be3fdd075860e815c72e35065/synthesizer/program/src/resources/credits.aleo#L976-L1000

Tool used

Manual Review

Recommendation

we should consider crediting this portion of microcredits to an address. For example:

credits.aleo::split

+ const FEE_RECEIVER_ADDRESS: address = "aleo1...";
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;
    // update fee_receiver account
+   get.or_use account[FEE_RECEIVER_ADDRESS] 0u64 into r6;
+   add r6 10_000u64 into r7;
+   set r7 into account[FEE_RECEIVER_ADDRESS];    
    // Output the first record.
    output r4 as credits.record;
    // Output the second record.
    output r5 as credits.record;

credits.aleo::fee_public

+ const FEE_RECEIVER_ADDRESS: address = "aleo1...";
finalize fee_public:
    // Input the sender's address.
    input r0 as address.public;
    // Input the total fee amount.
    input r1 as u64.public;
    // Retrieve the balance of the sender.
    // If `account[r0]` does not exist, `fee_public` is reverted.
    get account[r0] into r2;
    // Decrements `account[r0]` by `r1`.
    // If `r2 - r1` underflows, `fee_public` is reverted.
    sub r2 r1 into r3;
    // Updates the balance of the sender.
    set r3 into account[r0];

    // update fee_receiver account
+   get.or_use account[FEE_RECEIVER_ADDRESS] 0u64 into r4;
+   add r4 r1 into r5;
+   set r5 into account[FEE_RECEIVER_ADDRESS];

Duplicate of #17

joicygiore commented 6 days ago

Escalate

Quoting the sponsor's comments:

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.

It can be seen that verify.rs does not handle the issue of credits.aleo::split fees. When a user calls credits.aleo::split, it only deducts the corresponding funds from the caller's credit, and does not update the account information of the fee collector, which results in the loss of funds.

Quoting the sponsor's comments again:

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

Even if the fee is reduced directly as mentioned in the comments, this operation should be synchronized to the total supply, but there is still no relevant operation here. The internal fund account also faces problems

sherlock-admin3 commented 6 days ago

Escalate

Quoting the sponsor's comments:

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.

It can be seen that verify.rs does not handle the issue of credits.aleo::split fees. When a user calls credits.aleo::split, it only deducts the corresponding funds from the caller's credit, and does not update the account information of the fee collector, which results in the loss of funds.

Quoting the sponsor's comments again:

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

Even if the fee is reduced directly as mentioned in the comments, this operation should be synchronized to the total supply, but there is still no relevant operation here. The internal fund account also faces problems

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 commented 4 days ago

I don't understand what the escalation is for. Is it to remove the duplication?

joicygiore commented 4 days ago

I don't understand what the escalation is for. Is it to remove the duplication?

Hello sir, I think I expressed two different meanings. #17 is whether the user has the motivation to call, and #18 is the issue of fund handling after the call.

infosecual commented 3 days ago

Related to my comment in #17.

No fees in Aleo have a recipient. All fees are burned (whether they are split program fees or fees in fee commitments). You are correct that this reduces overall supply. This is known and by design. Supply is decreased by all fees on every transaction and are increased by block rewards to validators and provers. This is not a security issue. It actually makes the Aleo's economics more secure because it prevents collusion between validators running up the min priority fees. This is similar to how Ethereum's eip-1559 works except Aleo burns the entire fee instead of just part of it.

joicygiore commented 3 days ago

Related to my comment in #17.

No fees in Aleo have a recipient. All fees are burned (whether they are split program fees or fees in fee commitments). You are correct that this reduces overall supply. This is known and by design. Supply is decreased by all fees on every transaction and are increased by block rewards to validators and provers. This is not a security issue. It actually makes the Aleo's economics more secure because it prevents collusion between validators running up the min priority fees. This is similar to how Ethereum's eip-1559 works except Aleo burns the entire fee instead of just part of it.

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?

except Aleo burns the entire fee instead of just part of it

We should question unreasonable handling methods, of course we can't be sure whether this will have an impact in the future, so I just hope to get more clues. Finally, I accept all the decisions of the judge.There is nothing to argue about, leave it to the judge.

WangSecurity commented 2 days ago

Based on this comment, I believe this issue is invalid. The fees shouldn't be sent to any address, they're burnt, which is why fee-related methods don't update the recipient's account information.

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

WangSecurity commented 1 day ago

Result: Invalid Duplicate of #17

sherlock-admin2 commented 1 day ago

Escalations have been resolved successfully!

Escalation status: