ArkProjectNFTs / bridge

The ArkProject Bridge: seamless transfer of NFTs between ETH L1 & Starknet L2. Smart contracts, user-friendly interface, secure & efficient solution. Experience the future of NFT ownership today
https://bridge.arkproject.dev
Apache License 2.0
23 stars 20 forks source link

withdraw_auto_from_l1 escrow is not zeroed #226

Closed ptisserand closed 1 month ago

ptisserand commented 1 month ago

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.

od-hunter commented 1 month ago

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.

od-hunter commented 1 month ago

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.

onlydustapp[bot] commented 1 month ago

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

od-hunter commented 1 month ago

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

Thank you ser🫡

ptisserand commented 1 month ago

Hi @od-hunter any update on this ?

od-hunter commented 1 month ago

Hi @od-hunter any update on this ?

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