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

Forwarded Uris from L2 to L1 are not used

fyamf

Summary

When bridging from L2 to L1, if the associated collection on L1 is not present, it will be deployed and then the NFT will be minted. There are two issue regarding handling uris:

  • First: When deploying the NFT collection, the base uri is not set even if non-empty base uri is forwarded from L2.

  • Second: When minting the NFT, the token uri is not set even if non-empty token uris are forwarded from L2.

Thus, forwarded uris (base uri or token uris) from L2 are not playing any role when deploying/minting the NFT on L1.

Vulnerability Details

When bridging from L1 to L2, during withdrawal on L2, if the associated collection is not present, it will be deployed. During deployment, the base uri (which is provided from L1) will be set in the constructor.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L141
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L448
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/collection_manager.cairo#L146

#[constructor]
fn constructor(
ref self: ContractState,
name: ByteArray,
symbol: ByteArray,
base_uri: ByteArray,
bridge: ContractAddress,
collection_owner: ContractAddress,
) {
assert(!bridge.is_zero(), 'Invalid bridge address');
assert(!collection_owner.is_zero(), 'Bad collection owner address');
​
self.erc721.initializer(name, symbol, base_uri);
self.bridge.write(bridge);
self.ownable.initializer(collection_owner);
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L72

If NFT is not escrowed, it will be minted. If token uris are provided, then erc721_bridgeable will be deployed and the token uris will be set by calling mint_from_bridge_uri.

if (req.uris.len() != 0) {
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());

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

fn mint_from_bridge_uri(ref self: ContractState, to: ContractAddress, token_id: u256, token_uri: ByteArray) {
IERC721Bridgeable::mint_from_bridge(ref self, to, token_id);
self.token_uris.write(token_id, token_uri);
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L89

The issue is when bridging from L2 to L1:

When L2 to L1 bridging, during withdrawal on L1, if the associated collection is not present, it will be deployed. But during deployment, the base uri (which is provided from L2) is not used. It means that the newly-deployed ERC721Bridgeable will have empty base uri even if it is provided by L2.

collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);

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

Moreover, if NFT is not escrowed, it will be minted. But, even if token uris are provided by L2, they will not be used during minting when invoking the function mintFromBridge. It means that, the newly-minted NFT will have empty token uri even if it is provided by L2.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L208

Impact

  • Not using the uris forwarded from L2.

  • The newly-minted/deployed NFT on L1 will have empty uris.

Tools Used

Recommendations

The same mechanism implemented on L2, during bridging from L1 to L2, should be used to handle the forwarded uris from the source chain.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

invalid-NFT-minted-without-baseURI-URIs-or-bridge-with-no-metadata

URI is not lost on the origin chain and it can be modified with `ERC721UriImpl`. As explained in the TODO  below, that’s a design choice and it will be implemented as a future feature. https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L206 `ERC721Bridgable` is out of scope.

Support

FAQs

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