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

The `CollectionManager::_setL1L2AddressMapping` disrupts L1 To L2 Mappings such that two L1 addresses are mapped to one L2 address or two L2 addresses are mapped to one L1 address

spomaria

Summary

The CollectionManager::_setL1L2AddressMapping is designed to map one L1 address to an L2 address and similarly map that L2 address to the L1 address. Furthermore, if force is passed as True to CollectionManager::_setL1L2AddressMapping, this operation is carried out even if these addresses had any previous mappings before the function call.

Vulnerability Details

The vulnerability of this function lies in the fact that when the CollectionManager::_setL1L2AddressMapping function is called with force passed as True, previous mappings are only partially over-written since the new addresses are mapped without resetting any previous mappings in both ramifications. See https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/CollectionManager.sol#L151-L165

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();
}
}

Impact

Since the CollectionManager::_setL1L2AddressMapping function does not reset any previous mappings before mapping the two addresses together, this leads to a situation where two L1 addresses can be mapped to the same L2 address or two L2 addresses be mapped to the same L1 address. This can cause confussion to the users of the protocol.

Tools Used

Manual Review

Foundry

Proof of Concept:

  1. Call the CollectionManager::_deployERC721Bridgeable passing it an L2 address say collectionL2. This function will deploy a contract and map the contract address say L1Address to collectionL2 and vice versa. See https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/CollectionManager.sol#L49-L70

  2. Now call the CollectionManager::_setL1L2AddressMapping function passing it the following parameters: newL1Address, collectionL2 and True. This function will in turn map newL1Address to collectionL2 and also map collectionL2 to newL1Address. Note that this function does not reset any previous mappings whatsoever.

  3. Thus,

    • L1Address is mapped to collectionL2,

    • newL1Address is mapped to collectionL2, but

    • collectionL2 is only mapped to newL1Address
      The protocol now has two L1 addresses mapped to the same L2 address but the L2 address is only mapped to one of the two L1 addresses.

PoC Create a public version of the `CollectionManager` contract and add it to the `Escrow.t.sol` as follows:
import { CollectionManager } from "../src/token/CollectionManager.sol";
import "../src/sn/Cairo.sol";
.
.
.
​
// @audit-addition
// Contract to enable external calls to CollectionManager functions
contract CollectionManagerPublic is CollectionManager {
​
// type snaddress is uint256;
​
function deployERC721Bridgeable(
string memory name,
string memory symbol,
snaddress collectionL2,
uint256 reqHash
)
external
returns (address)
{
return _deployERC721Bridgeable(name , symbol, collectionL2,reqHash);
}
​
function setL1L2AddressMapping(
address collectionL1,
snaddress collectionL2,
bool force
)
external
{
_setL1L2AddressMapping(collectionL1, collectionL2, force);
}
​
function getL1AddressFromL2Address(snaddress _l2Address) public view returns(address l1Address) {
l1Address = _l2ToL1Addresses[_l2Address];
}
​
function getL2AddressFromL1Address(address _l1Address) public view returns(snaddress l2Address) {
l2Address = _l1ToL2Addresses[_l1Address];
}
}
​

Now, place the following code in the EscrowTest contract in the Escrow.t.sol script:

.
.
.
CollectionManagerPublic cmanager; // @audit-addition for setL1L2AddressMapping
.
.
​
function setUp() public {
cmanager = new CollectionManagerPublic(); // @audit-addition for setL1L2AddressMapping
.
.
.
}
​
function test_setL1L2AddressMappingDisruptsL1ToL2Mappings() public {
string memory name = "new NFT";
string memory symbol = "nNFT";
snaddress collectionL2 = snaddress.wrap(10);
uint256 reqHash = 1;
​
cmanager.deployERC721Bridgeable(name, symbol, collectionL2, reqHash);
​
// let's get the l1 and l2 addresses for the initial deployment
address initialL1Address = cmanager.getL1AddressFromL2Address(collectionL2);
snaddress initialL2Address = cmanager.getL2AddressFromL1Address(initialL1Address);
// show that l1 and l2 addresses are mapped
assertTrue(snaddress.unwrap(initialL2Address) == snaddress.unwrap(collectionL2));
​
// Now let's map collectionL2 to a new L1 address
address newL1Address = makeAddr("newL1Address");
bool force = true;
cmanager.setL1L2AddressMapping(newL1Address, collectionL2, force);
bool newL1AddressMappedToCollectionL2 = snaddress.unwrap(collectionL2) == snaddress.unwrap(cmanager.getL2AddressFromL1Address(newL1Address));
bool initialL1AddressMappedToCollectionL2 = snaddress.unwrap(collectionL2) == snaddress.unwrap(cmanager.getL2AddressFromL1Address(initialL1Address));
​
bool collectionL2IsMappedToNewL1Address = newL1Address == cmanager.getL1AddressFromL2Address(collectionL2);
bool collectionL2IsMappedToInitialL1Address = initialL1Address == cmanager.getL1AddressFromL2Address(collectionL2);
​
assert(newL1AddressMappedToCollectionL2);
assert(initialL1AddressMappedToCollectionL2);
assert(collectionL2IsMappedToNewL1Address);
assert(!collectionL2IsMappedToInitialL1Address);
}

Run: forge test --match-test test_setL1L2AddressMappingDisruptsL1ToL2Mappings -vvvv

Output:

.
.
.
├─ [23264] CollectionManagerPublic::setL1L2AddressMapping(newL1Address: [0xa23D323a6A253aE92A7389D664fD9376e041CFE1], 10, true)
│ └─ ← [Stop]
├─ [575] CollectionManagerPublic::getL2AddressFromL1Address(newL1Address: [0xa23D323a6A253aE92A7389D664fD9376e041CFE1]) [staticcall]
│ └─ ← [Return] 10
├─ [575] CollectionManagerPublic::getL2AddressFromL1Address(ERC1967Proxy: [0x037eDa3aDB1198021A9b2e88C22B464fD38db3f3]) [staticcall]
│ └─ ← [Return] 10
├─ [491] CollectionManagerPublic::getL1AddressFromL2Address(10) [staticcall]
│ └─ ← [Return] newL1Address: [0xa23D323a6A253aE92A7389D664fD9376e041CFE1]
├─ [491] CollectionManagerPublic::getL1AddressFromL2Address(10) [staticcall]
│ └─ ← [Return] newL1Address: [0xa23D323a6A253aE92A7389D664fD9376e041CFE1]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 155.10ms (39.67ms CPU time)

Recommendations

  1. Consider resetting any previous mappings before setting any new mappings in the CollectionManager::_setL1L2AddressMapping function as below

function _setL1L2AddressMapping(
address collectionL1,
snaddress collectionL2,
bool force
)
internal
{
if (((snaddress.unwrap(_l1ToL2Addresses[collectionL1]) == 0) && (_l2ToL1Addresses[collectionL2] == address(0)))
|| (force == true)) {
+ _l1ToL2Addresses[_l2ToL1Addresses[collectionL2]] = snaddress.wrap(uint256(0));
+ _l2ToL1Addresses[_l1ToL2Addresses[collectionL1]] = address(0);
​
_l1ToL2Addresses[collectionL1] = collectionL2;
_l2ToL1Addresses[collectionL2] = collectionL1;
} else {
revert CollectionMappingAlreadySet();
}
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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