ArkProject: NFT Bridge

Summary

NFTs can be stolen from escrow after bridging to L1

Vulnerability Details

when bridging from L2 - L1 you must call another function on L1 to finalize the bridging and send the nft to your wallet. The function is called withdrawTokens

the function withdrawTokens takes an array of uint256 as calldata and then will deserialize the inputs.

The problem occurs because the code never checks that the inputted owner address was the actual owner of the nft, instead we simply transfer the nft to that address. An attacker can easily input his own address to withdraw any nft token that is in the escrow.

from the function above we can see that the req.ownerL1 is a value that is arbitrary and it can be set to a the malicious user own address to send the nft to his address.

Therefore any of the tokens bridged from L2 to L1 can be stolen by a malicious actor. Below we see how we have a call to consumeMessageStarknet, this is where the problem is.

this bug is allowed because of a bug in encodePacked let us look at the function below from the starknet contracts

https://github.com/starkware-libs/cairo-lang/blob/4e233516f52477ad158bc81a86ec2760471c1b65/src/starkware/starknet/eth/StarknetMessaging.sol#L119C1-L126C11

the use of dynamic types in encode packed allows hash collisions to occur and thus a malicious user can submit a tx with different data but same hash as an innocent users tx and send his nft elsewhere. This is described in the solidity docs

If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred.

https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=collisions#non-standard-packed-mode

therefore the malicious actor may have different payload but same hash as victim and pass the consumeMessageFromL2 in order to send his nft to another address.

Lines of Code

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L153

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Escrow.sol#L63

Impact

Direct theft of user nft

Tools Used

manual reveiw

Recommendations

add a way to ensure ownly the rightful owner of the nft can withdraw said nft from escrow.
Otherwise the nft can be stolen by a malicious user.