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

withdraw_auto_from_l1 escrow is not zeroed #226

Open
ptisserand opened this issue Sep 25, 2024 · 7 comments · May be fixed by #246
Open

withdraw_auto_from_l1 escrow is not zeroed #226

ptisserand opened this issue Sep 25, 2024 · 7 comments · May be fixed by #246
Assignees
Labels
app:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8

Comments

@ptisserand
Copy link
Collaborator

ptisserand commented Sep 25, 2024

From https://codehawks.cyfrin.io/c/2024-07-ark-project/s/177

L2 escrow is not reset when withdrawn.

details

When tokens are deposited on L2 via deposit_tokens(), they will be deposited in the escrow mapping, so when someone transfers back the same NFTs from L1 → L2 to take it from the contract balance, because they already exist.

But when they are transferred back to L2, withdraw_auto_from_l1() transfers them if they exist in the escrow mapping, but never resets the mapping when transferring it, this creates confusion that the tokens are still in the bridge.cairo contract.

Unit test must be provided.

@ptisserand ptisserand added app:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8 labels Sep 25, 2024
@od-hunter
Copy link

od-hunter commented Sep 25, 2024

Hi @ptisserand , please can I be assigned this when od hack starts?

To solve this issue, I'd take the following steps:

  1. I'll update withdraw_auto_from_l1() (add logic to reset the escrow mapping after tokens are transferred, ensuring they are no longer recorded in the contract's state).
  2. I'll test that after depositing tokens, withdrawing them using withdraw_auto_from_l1() resets the escrow.
    Validate the escrow mapping is cleared after the withdrawal.
  3. I'll ensure tests cover multiple scenarios to confirm the escrow is correctly reset in all cases.

Please assign me.

@mubarak23
Copy link

mubarak23 commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a Experience Cairo smart contract developer with experience working on projects such as Just Art Peace, Dojo, Kart, TBA, and Shinigami. Before transitioning to Cairo development, I was a backend developer specializing in Rust.

My recent work with cairo starknet

My recent work with rust

How I plan on tackling this issue

As issue describe

  • reset the mapping after withdrawal is handle
  • test the case of making sure mapping is reset after withdrawal
  • test edge cases

ETH: 15HRS

@od-hunter
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Please can I be assigned this issue? This is my first time applying in this ecosystem and I’d love to be given the opportunity. I am a blockchain Developer, and my experience includes html, css, react, JavaScript,TypeScript and solidity, python and Cairo.

How I plan on tackling this issue

To solve this issue, I'd take the following steps:

1.⁠ ⁠I’ll update withdraw_auto_from_l1() (add logic to reset the escrow mapping after tokens are transferred, ensuring they are no longer recorded in the contract's state).
2.⁠ ⁠I’ll test that after depositing tokens, withdrawing them using withdraw_auto_from_l1() resets the escrow.
Validate the escrow mapping is cleared after the withdrawal.
3.⁠ ⁠I’ll ensure tests cover multiple scenarios to confirm the escrow is correctly reset in all cases.

Please assign me.

Copy link

onlydustapp bot commented Sep 26, 2024

The maintainer ptisserand has assigned od-hunter to this issue via OnlyDust Platform.
Good luck!

@od-hunter
Copy link

The maintainer ptisserand has assigned od-hunter to this issue via OnlyDust Platform. Good luck!

Thank you ser🫡

@ptisserand
Copy link
Collaborator Author

Hi @od-hunter any update on this ?

@od-hunter
Copy link

Hi @od-hunter any update on this ?

I’ve started working on it. Will push a pr soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:cairo Work on the Starknet part of the application; you need to know Cairo lang. ODHack8
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants