ArkProject: NFT Bridge

Summary

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.
I want to note that currently the new_owners/newOwners variable is unused but it is expected to be introduced as noted by a TODO in the code. Since this is currently not exploitable but will be once the feature is implemented as written in the TODO, I believe the likelihood of this is low/medium.
Since the impact of this is very high and I would assign it a high severity if the feature were to be implemented, I decided on the middleground of giving it a medium severity.

Since the impact of this is very high and I would assign it a high severity if the feature were to be implemented, I decided on the middleground of giving it a medium severity.

Vulnerability Details

The problem is, that the new_owners (on L2) and newOwners (on L1) variables have conflicting types. Concretely, the type on L2 is Span<ContractAddress> while on L1 it is uint256[].
This can be seen when we compare the Request struct in the Protocol.sol and request.cairo files.
Now this is a problem because the proper equivalent type for a ContractAddress on L2 is an address on L1. Where this discrepancy becomes a problem is when we deserialize ContractAddress <-> uint256.

If we go through the deserialization of a ContractAddress to a uint256, we see this more concretely:

A ContractAddress on L2 gets serialized to a single uint256 field in the resulting buffer on L1. This can be seen in requestDeserialize in Protocol.sol:

In order to deserialize we just take the buf at an offset and convert it to an address.

But what if we now interpret buf[offset] as a uint256?

As we can see, this is done in the same function a little further down:

Here we take the buf, at the offset where we expect the newOwners array to be, as a uint256[].
Now since fields of uint256 are serialized as two fields, one containing low and the next containing high, we now take the next two fields for the first newOwner instead of only one.

Let's visualize this:

• assume we get two newOwners on L2, we serialize them to the following, since they fit into one felt252 each:

  • [prev stuff]

  • [owner1]

  • [owner2]

• if we now take this request buffer on L1 and try to deserialize it, we first deserialize all the previous data and then get to the newOwners

• since we think each owner is a uint256, we deserialize from a buffer buf (assuming offset is 0) like this:

• buf[owner1, owner2] -> (buf[1] << 128) | uint128(buf[0]) (as seen in uint256Deserialize in Cairo.sol)

• this means we just took our two newOwners and put them into one single uint256 which is obviously corrupted now and not a valid L1 address

• therefore this call will fail further down the line

• since this will not revert the L2 side's call to send_message_to_l1_syscall, this will cause the bridged NFT to be permanently stuck in the bridge as it cannot be transfered out of the bridge as the receiver address is completely corrupted

Impact

Direct loss of funds (NFTs) for the user

Tools Used

Manual review

Recommended Mitigation

In order to prevent this, I would recommend adjusting the type in the Request struct in Protocol.sol from uint256[] to address[] and deserialize it accordingly. This would prevent this issue entirely if the deserialization is done properly.