NFTs can be lost due to mismatched types if `newOwners/new_owners` is implemented

[ptisserand]

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

There is currently a mismatch of types between the L1 and L2 implementations of the Request struct. This will cause the bridge to be unusable with the new_owners/newOwners feature and lead to loss of NFTs.

This issue is to fix new_owners/newOwners feature for next version.

Unit test must be provided.

[PavitraAgarwal21]

I’ve read the bug report on Codehawks and understand the vulnerability. Please assign it to me when ODHack starts .

[SoarinSkySagar]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

GM, I am Sagar Rana, a smart contract developer and full stack engineer. I have 3 years of experience building robust full stack applications and over a year of writing smart contracts. You can see my projects and contributions to some major repos on my GitHub profile. The tech stack I use mainly includes Solidity, Rust, JavaScript and Typescript. I am also contributing to the Starknet and Rust ecosystems and building on Cairo and Rust languages. I am interested in contributing to projects like this to learn more about these technologies and help make these projects better. Please assign me as I would be really glad to be a contributor in this project! :)

How I plan on tackling this issue

Hi @ptisserand, I have gone through the codehawks article and this issue can be approached by:

• Update the Request struct in Protocol.sol. Instead of using uint256[] for newOwners, use address[].
• Update the relevant logic for this change:

  (address[] memory newOwners, ) = Cairo.addressArrayDeserialize(buf, offset);

• Update the Deserialization Logic in Protocol.sol
• Update the Cairo code serialization logic: The ContractAddress in Cairo is represented by felt252, which corresponds to 160-bit addresses in the solidity file. Need to ensure that when new_owners is serialized, ContractAddress fits into felt252
• Test the changes for any errors

Tasks:

• [ ] Modify Solidity code to ensure the correct type is implemented
• [ ] Change the deserialization logic in Solidity
• [ ] Change Deserialization logic in Cairo file

ETA: 1 Day

[aji70]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

i'm a solidity and cairo smart contract developer with over 2 years experience and believe i have the skill set for the task and i am also very good with smart contract testing

How I plan on tackling this issue

i would check both stucts snd correct the mismatches

[onlydustapp[bot]]

The maintainer ptisserand has assigned SoarinSkySagar to this issue via OnlyDust Platform. Good luck!

[SoarinSkySagar]

Hi @ptisserand, I have completed all the required tasks and ensured good coding practices in the commit (PR #235), please review and merge!