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

Cannot Withdraw token because of `ErrorVerifyingAddressMapping` error

danzero

Summary

Lack of logic in function _verifyRequestAddressesin CollectionManager.sol causes tokens that has no mapping cannot be withdrawed unless the mapping is updated by the owner.

Vulnerability Details

Users withdraw their token usnig the withdrawTokensfunction in Bridge.sol. Some checks and verification are performed, 1 of them is the _verifyRequestAddressesin CollectionManager.sol. Basically this function checks whether the _l1ToL2Addresses mapping contains the L1 collection address and L2 collection address requested by the user as seen on the code below.

function _verifyRequestAddresses(
address collectionL1Req,
snaddress collectionL2Req
)
internal
view
returns (address)
{
address l1Req = collectionL1Req;
uint256 l2Req = snaddress.unwrap(collectionL2Req);
address l1Mapping = _l2ToL1Addresses[collectionL2Req];
uint256 l2Mapping = snaddress.unwrap(_l1ToL2Addresses[l1Req]);
​
// L2 address is present in the request and L1 address is not.
if (l2Req > 0 && l1Req == address(0)) {
if (l1Mapping == address(0)) {
// It's the first token of the collection to be bridged.
return address(0);
} else {
// It's not the first token of the collection to be bridged,
// and the collection tokens were only bridged L2->L1.
return l1Mapping;
}
}
​
// L2 address is present, and L1 address too.
if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {
revert InvalidCollectionL1Address();
} else if (l2Mapping != l2Req) {
revert InvalidCollectionL2Address();
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}
​
revert ErrorVerifyingAddressMapping();

If the _l1ToL2Addresses mapping do not contains the requested L1 collection address then the user cannot withdraw the requested tokens. Looking at the function it only account for scenarios where:

  • L2 collection address is present L1 collection address is present

  • L2 collection address is present L1 collection address is not

But not when L1 collection address is present and L2 collection address is not.

This should not be a problem if users cannot deposit their token in the first place if the mapping does not contains the requested L1 collection address. But the depositTokensfunction in Bridge.solactually allows any ERC721 token to be deposited even if the mapping do not contain the L1 collection address yet if the owner has not enabled whitelisting through enableWhitelistfunction.

POC

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
​
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC1155/IERC1155.sol";
​
import "forge-std/Test.sol";
import "forge-std/console.sol";
​
import "../src/IStarklane.sol";
import "../src/IStarklaneEvent.sol";
import "../src/State.sol";
​
import "../src/Bridge.sol";
import "../src/sn/Cairo.sol";
import "../src/sn/StarknetMessagingLocal.sol";
import "../src/sn/IStarknetMessagingLocal.sol";
import "../src/token/Deployer.sol";
import "../src/token/TokenUtil.sol";
​
import "./utils/Users.sol";
import "./token/ERC721MintFree.sol";
import "./token/IERC721MintRangeFree.sol";
​
contract Hack is Test {
​
address bridge;
address erc721C1;
address erc1155C1;
address snCore;
​
address payable internal alice;
address payable internal bob;
​
function computeMessageHashFromL2(
uint256[] memory request
)
public
returns (bytes32)
{
(snaddress starklaneL2Address, felt252 starklaneL2Selector)
= IStarklane(bridge).l2Info();
​
// To remove warning. Is there a better way?
assertTrue(felt252.unwrap(starklaneL2Selector) > 0);
​
bytes32 msgHash = keccak256(
abi.encodePacked(
snaddress.unwrap(starklaneL2Address),
uint256(uint160(bridge)),
request.length,
request)
);
​
return msgHash;
}
​
function testDepositSuccessWithdrawFail() public{
//init
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
​
erc721C1 = address(new ERC721MintFree("name 1", "S1"));
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](3);
ids[0]=1;
ids[1]=2;
ids[2]=0;
​
snCore = address(new StarknetMessagingLocal());
address impl = address(new Starklane());
bytes memory dataInit = abi.encodeWithSelector(
Starklane.initialize.selector,
abi.encode(
address(this),
snCore,
0x1,
0x2
)
);
​
bridge = address(new ERC1967Proxy(impl, dataInit));
IStarklane(bridge).enableBridge(true);
//IStarklane(bridge).setL1L2CollectionMapping(address(erc721C1), Cairo.snaddressWrap(0x123), true);
​
console.log("This: ", address(this));
console.log("Bridge: ", bridge);
console.log("SNCore: ", snCore);
console.log("Impl: ", impl);
console.log("ERC721C1: ", erc721C1);
console.log("Alice: " , alice);
console.log("Bob: " , bob);
​
//deposit
vm.startPrank(alice);
​
console.log("DEPOSIT");
console.log("---------------------------------------");
console.log(ERC721(erc721C1).ownerOf(0));
console.log(ERC721(erc721C1).ownerOf(1));
console.log(ERC721(erc721C1).ownerOf(2));
​
​
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
​
​
IStarklane(bridge).depositTokens{value: 1}(
salt,
address(erc721C1),
to,
ids,
false
);
​
console.log("WITHDRAW");
console.log("---------------------------------------");
console.log(ERC721(erc721C1).ownerOf(0));
console.log(ERC721(erc721C1).ownerOf(1));
console.log(ERC721(erc721C1).ownerOf(2));
​
vm.stopPrank();
​
//withdraw
vm.startPrank(alice);
​
uint256[] memory values = new uint256[](0);
string[] memory uris = new string[](0);
uint256[] memory newOwners = new uint256[](0);
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
​
Request memory reqFull = Request ({
header: header,
hash: 0x0,
collectionL1: erc721C1,
collectionL2: Cairo.snaddressWrap(0x123),
ownerL1: alice,
ownerL2: Cairo.snaddressWrap(0x0),
name: "Good",
symbol: "GOOD",
uri: "URIURI",
tokenIds: ids,
tokenValues: values,
tokenURIs: uris,
newOwners: newOwners
});
​
uint256[] memory reqSerialized = Protocol.requestSerialize(reqFull);
​
uint256[] memory msgHashes = new uint256[](1);
msgHashes[0] = uint256( computeMessageHashFromL2(reqSerialized) );
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(msgHashes);
IStarklane(bridge).withdrawTokens(reqSerialized);
​
console.log("---------------------------------------");
console.log(ERC721(erc721C1).ownerOf(0));
console.log(ERC721(erc721C1).ownerOf(1));
console.log(ERC721(erc721C1).ownerOf(2));
}
​
​
}
​

Uncomment line 88 to simulate the owner set the mapping for the L1 collection

Impact

Users are unable to withdraw their tokens until the mapping is updated by the owner, which poses a significant problem if users wish to change their minds and transfer their tokens elsewhere. While the owner can resolve the issue by updating the mapping, there is no assurance regarding the timeframe for this update. Given the vast number of L1 collection addresses out there, it is impractical for the owner to update the mapping for every address out there. Additionally, this could lead to reputational damage if a user affected by this issue accuses the protocol or project of theft and the accusation gains traction on social media.

Tools Used

Manual review

Foundry

Recommendations

Do not allow deposit of token that has no mapping so that users does not accidentally lock their tokens in the escrow using requirestatement somewhere in the depositTokensfunction in Bridge.sol, something like this:

require( snaddress.unwrap(_l1ToL2Addresses[collectionL1]) != 0, "No mapping exist yet for this collection")
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-first-bridgeof-a-collection-L1<->L2-do-not-sync-addresses

Likelyhood: High, any collections bridged, without bridge owner action, will be unable to bridge back. Impact: High, L2 -> L1 tokens will be stuck in the bridge. L1 -> L2 will need to ask for a cancellation.

Support

FAQs

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