Skip to content
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.

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

feat(token portal standard): create token portal standard and corresponding cross chain test #2293

Closed
wants to merge 3 commits into from

Conversation

rahul-kothari
Copy link
Contributor

@rahul-kothari rahul-kothari commented Sep 14, 2023

Part of #2167. In Future PRs, I aim to

@rahul-kothari rahul-kothari changed the title token portal standard and corresponding cross chain test feat(token portal standard): and corresponding cross chain test Sep 14, 2023
@rahul-kothari rahul-kothari changed the title feat(token portal standard): and corresponding cross chain test feat(token portal standard): create token portal standard and corresponding cross chain test Sep 14, 2023
Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

Haven't mainly read the main.nr contract but it is unsecure and need some changes.

@@ -0,0 +1,43 @@
struct AztecAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably but those somewhere separate such that it is not duplicated across the token and here.

}

// Computes a content hash of a deposit/mint message.
fn get_mint_content_hash(amount: Field, owner_address: Field, canceller: Field) -> Field {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ambiguous, since the same message is used for two functions, I can break your assumptions around it. Example: say that you want to do a public deposit into a contract, I see it, and can then consume that message to be a private mint instead with a known secret which I can then take myself.

// User needs to call token.redeem_shield() to get the private assets
// This method is public because it accesses public storage. For similar reasons, the corresponding call on the token is also public
#[aztec(public)]
fn mint(
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned below. I can hijack your public deposit and steal the funds, by consuming it with a mint instead of the intended mint_public.

let storage = Storage::init(Context::public(&mut context));

// Burn tokens on L2
let return_value = Token::at(storage.token.read()).burn_public(context, from.address, amount, nonce);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be very dangerous if you are not inserting the approval in a batch where you instantly consume it.
Anyone can jump in after you made that approval and consume it but send the funds to themselves instead.

To mitigate, remove the from and use context.msg_sender() instead, or add another authwit check for the more complete action.

// Burns the appropriate amount of tokens and creates a L2 to L1 withdraw message privately
// Requires `from` to give approval to the bridge to burn tokens on their behalf using witness signatures
#[aztec(private)]
fn withdraw(
Copy link
Contributor

Choose a reason for hiding this comment

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

Re above.

In private it is less dangerous since it requires that

  • the attacker has your privacy key
  • you provided him with the signature to spend the funds as well

@LHerskind LHerskind mentioned this pull request Sep 15, 2023
4 tasks
@rahul-kothari rahul-kothari force-pushed the rk/token_portal_standard2 branch 4 times, most recently from 1c371d7 to 1bb8f3f Compare September 15, 2023 11:18
@ludamad ludamad deleted the rk/token_portal_standard2 branch August 22, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants