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

Accidental L1 L2 address mapping update can lead to asset lockdown

danzero

Summary

The owner can accidentaly input address(0) as one of the mapping.

Vulnerability Details

These 2 state variables in CollectionManager.sol store the mapping between the L1 and L2 collections addresses:

// Mapping between L2<->L1 contracts addresses.
mapping(snaddress => address) _l2ToL1Addresses;
​
// Mapping between L1<->L2 contracts addresses.
mapping(address => snaddress) _l1ToL2Addresses;

The owner can change/update the mapping through this external function which in turn call the internal function below this code block:

// in Bridge.sol
function setL1L2CollectionMapping(
address collectionL1,
snaddress collectionL2,
bool force
) external onlyOwner {
_setL1L2AddressMapping(collectionL1, collectionL2, force);
emit L1L2CollectionMappingUpdated(collectionL1, snaddress.unwrap(collectionL2));
}
​
// in CollectionManager.sol
function _setL1L2AddressMapping(
address collectionL1,
snaddress collectionL2,
bool force
)
internal
{
if (((snaddress.unwrap(_l1ToL2Addresses[collectionL1]) == 0) && (_l2ToL1Addresses[collectionL2] == address(0)))
|| (force == true)) {
_l1ToL2Addresses[collectionL1] = collectionL2;
_l2ToL1Addresses[collectionL2] = collectionL1;
} else {
revert CollectionMappingAlreadySet();
}
}

Both of the functions does not contain any check/validation for zero addresses. If the owner unintentionally change 1 of the existing collection address mapping with zero address, this could cause accidental mapping update that could lead to asset lockdown, as users currently can deposit any tokens but cannot withdraw it if the mapping do not contains the collection address.

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 testAccidentalMappingUpdate() 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();
​
//ACCIDENTAL MAPPING UPDATE
IStarklane(bridge).setL1L2CollectionMapping(address(erc721C1), Cairo.snaddressWrap(0x0), true);
// IStarklane(bridge).setL1L2CollectionMapping(address(0), Cairo.snaddressWrap(0x123), true);
​
//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));
}
​
​
}
​

Impact

  • Accidental mapping update can lead to unintentional asset lockdown

  • Reputational damage

Tools Used

Manual review

Foundry

Recommendations

Update the _setL1L2AddressMapping function in CollectionManager.sol to disallow changing existing mapping address with zero address.

function _setL1L2AddressMapping(
address collectionL1,
snaddress collectionL2,
bool force
)
internal
{
require( collectionL1 != address(0) && snaddress.unwrap(collectionL2) != 0, "One of the requested collection L1 or L2 address is zero address !");
if (((snaddress.unwrap(_l1ToL2Addresses[collectionL1]) == 0) && (_l2ToL1Addresses[collectionL2] == address(0)))
|| (force == true)) {
_l1ToL2Addresses[collectionL1] = collectionL2;
_l2ToL1Addresses[collectionL2] = collectionL1;
} else {
revert CollectionMappingAlreadySet();
}
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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