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

`_safeMint` should be used instead of `_mint` in `mintFromBridge`

x18a6

Summary

erc721_bridgeable::mint_from_bridge(...) function uses _mint instead of _safe_mint, tokens will be locked for contracts that cannot handle them i.e, that don't support the IERC721Receiver interface.

Vulnerability Details

starknet/src/token/erc721_bridgeable.cairo#L78-L93

#[abi(embed_v0)]
impl ERC721BridgeableImpl of IERC721Bridgeable<ContractState> {
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); //@audit should this be cleared to clean token.uris?
}
}

The problem is that _mint does not check if to (i.e req.owner_l2) can handle ERC721 tokens, which is used in Bridge::withdraw_tokens(...) to withdraw tokens from the bridge:

starknet/src/bridge.cairo#L165-L169

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
...
let to = req.owner_l2;
let from = starknet::get_contract_address();
​
let is_escrowed = !self.escrow.read((collection_l2, token_id)).is_zero();
​
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
​
} else {
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());
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
@>> .mint_from_bridge(to, token_id);
}
}
​
i += 1;
};
...
}

If req.ownerL2 is a smart contract that does not implement IERC721Receiver, tokens will be locked in the bridge. Same goes for tokens in the solidity contract (We understand token/ERC721Bridgeable.sol is OOS, but it affects within-scope logic that could lock tokens).

For context on how is _mint different from _safeMint, see ERC721/ERC721.sol#L226-L238 & ERC721/ERC721.sol#L252-L286.

Impact

Tokens locked.

Tools Used

Manual review.

Recommendations

#[abi(embed_v0)]
impl ERC721BridgeableImpl of IERC721Bridgeable<ContractState> {
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._safe_mint(to, token_id);
- self.erc721._mint(to, token_id);
​
}
...
function mintFromBridge(address to, uint256 id) public onlyOwner {
+ _safeMint(to, id);
- _mint(to, id);
}
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.