NFTBridge
60,000 USDC
View results
Submission Details
Severity: low
Invalid

No using safe_transfer_from and safe_mint in Cairo & Solidity contracts can lead to loss of NFTs

Summary

The Cairo and Solidity bridge implementations do not utilize the safe_transfer_from/safeTransferFrom and safe_mint/safeMint functions, which can lead to the loss of NFTs if they are sent to smart contracts that do not support the ERC721 and ERC1155 receiver interfaces.

Vulnerability Details

In the provided code:

  • Solidity Contracts:

    • mintFromBridge functions in the Solidity contracts use _mint directly instead of safeMint. This bypasses the checks that safeMint would typically perform to ensure that the recipient contract can handle the received tokens.

    function mintFromBridge(
    address to,
    uint256 id,
    uint256 value
    )
    public
    onlyOwner
    {
    _mint(to, id, value, "");
    }
    function mintFromBridge(address to, uint256 id)
    public
    onlyOwner {
    _mint(to, id);
    }
  • Cairo Contracts:

    • The Cairo implementation uses _mint for minting operations and transfer_from for transferring tokens, which do not include checks for ERC721 and ERC1155 receiver interfaces.

    fn mint_from_bridge(ref self: ContractState, to: ContractAddress, token_id: u256) {
    assert(
    starknet::get_caller_address() == self.bridge.read(),
    'ERC721: only bridge can mint'
    );
    self.erc721._mint(to, token_id);
    }
    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);
    }
    fn escrow_deposit_tokens(
    ref self: ContractState,
    contract_address: ContractAddress,
    from: ContractAddress,
    token_ids: Span<u256>,
    ) {
    let to = starknet::get_contract_address();
    let erc721 = IERC721Dispatcher { contract_address };
    let mut i = 0_usize;
    loop {
    if i == token_ids.len() {
    break ();
    }
    let token_id = *token_ids[i];
    erc721.transfer_from(from, to, token_id);
    self.escrow.write((contract_address, token_id), from);
    i += 1;
    };
    }

Impact

  1. Transaction Failures: NFT transfers or mints to smart contracts that do not support the ERC721 or ERC1155 receiver interfaces will fail. This results in a loss of tokens if the recipient contract cannot handle the transfer properly.

  2. Token Loss: NFTs sent to contracts that do not implement the necessary receiver functions may be lost forever, as they are not received by the intended destination and cannot be retrieved.

  3. Reduced Usability: Users and developers may face issues when interacting with the bridge, as the expected functionality to transfer or mint tokens safely is not enforced.

Recommendations

  1. Use Safe Transfer and Mint Functions: Implement the use of safeTransferFrom and safeMint functions in the Solidity and Cairo contracts to ensure that tokens are only transferred to contracts that can handle them properly.

    Solidity Example:

    function mintFromBridge(
    address to,
    uint256 id,
    uint256 value
    )
    public
    onlyOwner
    {
    // Use safe minting function
    _safeMint(to, id, value, "");
    }
    function mintFromBridge(address to, uint256 id)
    public
    onlyOwner {
    // Use safe minting function
    _safeMint(to, id);
    }

    Cairo Example:

    fn mint_from_bridge(ref self: ContractState, to: ContractAddress, token_id: u256) {
    // Implement safe mint logic
    assert(
    starknet::get_caller_address() == self.bridge.read(),
    'ERC721: only bridge can mint'
    );
    // Safe mint function to ensure the recipient contract can handle the token
    self.erc721.safe_mint(to, token_id);
    }
    fn escrow_deposit_tokens(
    ref self: ContractState,
    contract_address: ContractAddress,
    from: ContractAddress,
    token_ids: Span<u256>,
    ) {
    let to = starknet::get_contract_address();
    let erc721 = IERC721Dispatcher { contract_address };
    let mut i = 0_usize;
    loop {
    if i == token_ids.len() {
    break ();
    }
    let token_id = *token_ids[i];
    // Use safe transfer function
    erc721.safe_transfer_from(from, to, token_id);
    self.escrow.write((contract_address, token_id), from);
    i += 1;
    };
    }
  2. Add Receiver Interface Checks: Ensure that the receiving contracts implement the necessary ERC721 and ERC1155 receiver interfaces before performing transfers or mints.

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.