ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Asset can be locked on L1 if L2 owner is set to zero

sancybars

Summary

The deposit() function of the L1NftBridge(`starkLane`) contract allows users to deposit with the `ownerL2`address set to 0.

The only validation done is to ensure no overflow occurs in the "net" by checking the Val against the SN_MODULOU`.

function isFelt252(
​
uint256 val
​
)
​
internal
​
pure
​
returns (bool)
​
{
​
return val < SN\_MODULUS;
​
}
​

However In StarkNet users do not have addresses. Transactions sent to the network have the 0 address as caller.

In order to identify accounts via addresses, each user deploys their account contract and interacts with

contracts such as the `starkLane` used to bridge the Nfts using their account-contract.

The execution of `finalize_deposit` initiated by the `l1_handler` on l2 however will fail as sending or minting for the zero address will revert. As a result the deposited Nfts on L1 will be locked in

the escrow.

Vulnerability Details

function depositTokens(
​
uint256 salt,
​
address collectionL1,
​
snaddress ownerL2,
​
uint256\[] calldata ids,
​
bool useAutoBurn
​
)
​
external
​
payable
​
{
​
if (!Cairo.isFelt252(snaddress.unwrap(ownerL2))) {
​
revert CairoWrapError();
​
}
​
if (!\_enabled) {
​
revert BridgeNotEnabledError();
​
}
​
​
​
CollectionType ctype = TokenUtil.detectInterface(collectionL1);
​
if (ctype == CollectionType.ERC1155) {
​
revert NotSupportedYetError();
​
}
​
​
​
if (!\_isWhiteListed(collectionL1)) {
​
revert NotWhiteListedError();
​
}
​
​
​
Request memory req;
​
​
​
// The withdraw auto is only available for request originated from
​
// Starknet side as the withdraw on starknet is automatically done
​
// by the sequencer.
​
req.header = Protocol.requestHeaderV1(ctype, useAutoBurn, false);
​
req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids);
​
// TODO: store request hash in storage to avoid replay attack.
​
// or can it be safe to use block timestamp? Not sure as
​
// several tx may have the exact same block.
​
req.collectionL1 = collectionL1;
​
req.collectionL2 = \_l1ToL2Addresses\[collectionL1];
​
​
​
req.ownerL1 = msg.sender;
​
req.ownerL2 = ownerL2;
​
​
​
if (ctype == CollectionType.ERC721) {
​
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil.erc721Metadata(
​
collectionL1,
​
ids
​
);
​
} else {
​
(req.uri) = TokenUtil.erc1155Metadata(collectionL1);
​
}
​
​
​
\_depositIntoEscrow(ctype, collectionL1, ids);
​
​
​
req.tokenIds = ids;
​
​
​
uint256\[] memory payload = Protocol.requestSerialize(req);
​
if (payload.length >= MAX\_PAYLOAD\_LENGTH) {
​
revert TooManyTokensError();
​
}
​
IStarknetMessaging(\_starknetCoreAddress).sendMessageToL2{value: msg.value}(
​
snaddress.unwrap(\_starklaneL2Address),
​
felt252.unwrap(\_starklaneL2Selector),
​
payload
​
);
​
​
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
​
}
​

Assets are first deposited then locked in escrow, which will be retrieved in L2 by minting the same collectionTypeand idto the ownerL2, however if the the ownerL2is set to zero the NFt won't be minted as minting to the zero address is restricted on L2.

Impact

Malicious users can exploit the vulnerability to lock NFTs in escrow, preventing legitimate buyers from acquiring them if being sold to users on L2.

Deposited assets on L1 will be locked in escrow.

Tools Used

Manual Review

Recommendations

ownerL2 != 0 to ensure that the address is non-zero.

Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.