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

withdrawTokens function in Bridge.sol does not check whether token being withdrawn is still whitelisted

udayj

Summary

withdrawTokens function in Bridge.sol does not check whether token being withdrawn is still whitelisted. This is because whitelisting of tokens on L1 and L2 sides is done independently and hence 1 side of the bridge might temporarily be whitelisted while the other side isnt.

Vulnerability Details

withdrawTokens does not check whether an existing collection_l1 is still whitelisted before processing the transaction. If it is a new collection on L1 side, then it is deployed and whitelisted
https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L183

But if it already exists, then the call to safeTransferFrom is made from _withdrawFromEscrow function without any check for whitelist.
But this call might go to a compromised contract.

function test_depositWithdrawTokens_withMapping() public {
// add mapping L1 <-> L2: erc721C1 <-> 0x123
IStarklane(bridge).setL1L2CollectionMapping(address(erc721C1), Cairo.snaddressWrap(0x123), true);
​
// alice deposit token 0 and 9 of collection erc721C1 to bridge
test_depositTokenERC721();
////////////////
IStarklane(bridge).whiteList(address(erc721C1), false); // HERE WHITELISTING IS REMOVED
////////////////
// Build the request and compute it's "would be" message hash.
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory req = buildRequestDeploy(header, 9, bob);
req.collectionL1 = address(address(erc721C1));
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
​
// The message must be simulated to come from starknet verifier contract
// on L1 and pushed to starknet core.
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
address collection = IStarklane(bridge).withdrawTokens(reqSerialized);
​
// TODO: add verification of event emission.
assertEq(collection, erc721C1);
assertEq(IERC721(erc721C1).ownerOf(9), bob);
​
// Error message from Starknet Core expected.
vm.expectRevert(bytes("INVALID_MESSAGE_TO_CONSUME"));
IStarklane(bridge).withdrawTokens(reqSerialized);
}

In the test above the whitelisting was removed for the collection_l1

Running forge test test_depositWithdrawTokens_withMapping shows the test passing and confirms the finding.

Impact

The impact could be severe and result in loss of NFT for receiver if the collection was removed from whitelist on L1 side due to the collection being compromised in any way.

Tools Used

Manual review

Recommendations

Changes to the whitelist should be made only from 1 side of the bridge through Starknet Messaging call, preferrably from L1 side since the call on L2 side would be synchronous without requiring additional actions from the admin. This will ensure there is only 1 source of truth for such data. Alternatively, the withdrawTokens function could check if collection_l1 is whitelisted and revert if it isnt whitelisted. But this would mean that the user's NFT gets stuck in escrow on L2 side and new code has to be developed to retrieve such stuck NFT through admin action.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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