ArkProject: NFT Bridge

Summary

The user can lead the NFTs to be transferred to a null address. Both from L1 to L2 and L2 to L1 depositTokens functions lack the null address or zero address check.

Vulnerability Details

The L1 depositTokens function has the sanity check for a valid starknet address but not for a null address and vice-versa for L2. As both functions are external user input sanitation lacks where neither L1 checks before making the transfer and nor L2 makes before minting the token. Furthermore, if the user tries to deliberately send the tokens to a null address, the bridge should stop the potential burn as this kind of functionality is not in the bridge principle or feature.

depositToken in Starknet (L2)

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L242-L306

depositToken in Ethereum (L1)

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L78-L144

Test cases written for the sanity of valid starknet address can be modified to pass the valid L2 zero address.

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/test/Cairo.t.sol#L17-L21

Impact

NFTs/ERC721 sent to the null address are lost permanently and lost for the user. Likelihood minimum and impact high.

Tools Used

Manual

Recommendations

Add checks for null addresses on both sides of the deposit function and keep an equal number of validations too.