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

Unchecked Forced Mapping Overwrite in Starklane Contract

juggernaut63

Summary

The setL1L2CollectionMapping function in the Starklane contract allows the owner to forcibly overwrite existing L1-L2 collection mappings without adequate validation. This can lead to unintended alterations of critical mappings.

Vulnerability Details

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L368-L375

  1. Setup Initial Mapping:

    • Call setL1L2CollectionMapping(collectionL1, collectionL2, false) to establish an initial mapping.

  2. Attempt Overwrite Without Force:

    • Call setL1L2CollectionMapping(collectionL1, newCollectionL2, false).

    • Observe revert due to CollectionMappingAlreadySet.

  3. Overwrite With Force:

    • Call setL1L2CollectionMapping(collectionL1, newCollectionL2, true).

    • Mapping is overwritten without additional checks.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "../src/Bridge.sol";
​
contract BridgeTest is Test {
Starklane bridge;
address owner = address(1);
address collectionL1 = address(2);
snaddress collectionL2 = snaddress.wrap(3);
​
function setUp() public {
bridge = new Starklane();
bytes memory data = abi.encode(owner, IStarknetMessaging(address(0)), uint256(0), uint256(0));
vm.prank(owner);
bridge.initialize(data);
vm.prank(owner);
bridge.enableBridge(true);
}
​
function testSetL1L2CollectionMappingWithForce() public {
vm.prank(owner);
bridge.setL1L2CollectionMapping(collectionL1, collectionL2, false);
​
snaddress newCollectionL2 = snaddress.wrap(4);
vm.prank(owner);
vm.expectRevert(CollectionMappingAlreadySet.selector);
bridge.setL1L2CollectionMapping(collectionL1, newCollectionL2, false);
​
vm.prank(owner);
bridge.setL1L2CollectionMapping(collectionL1, newCollectionL2, true);
}
}
forge test --match-path test/BridgeTest.t.sol -vvvv
[⠊] Compiling...
[⠆] Compiling 1 files with Solc 0.8.26
[⠔] Solc 0.8.26 finished in 2.26s
Compiler run successful with warnings:
Warning (5667): Unused function parameter. Remove or comment out the variable name to silence this warning.
--> src/token/TokenUtil.sol:114:9:
|
114 | address collection
| ^^^^^^^^^^^^^^^^^^
​
Warning (2018): Function state mutability can be restricted to pure
--> src/token/TokenUtil.sol:113:5:
|
113 | function erc1155Metadata(
| ^ (Relevant source part starts here and spans across multiple lines).
​
​
Ran 1 test for test/BridgeTest.t.sol:BridgeTest
[PASS] testSetL1L2CollectionMappingWithForce() (gas: 91719)
Traces:
[91719] BridgeTest::testSetL1L2CollectionMappingWithForce()
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [48975] Starklane::setL1L2CollectionMapping(SHA-256: [0x0000000000000000000000000000000000000002], 3, false)
│ ├─ emit L1L2CollectionMappingUpdated(colllectionL1: SHA-256: [0x0000000000000000000000000000000000000002], collectionL2: 3)
│ └─ ← [Stop]
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [0] VM::expectRevert(CollectionMappingAlreadySet())
│ └─ ← [Return]
├─ [967] Starklane::setL1L2CollectionMapping(SHA-256: [0x0000000000000000000000000000000000000002], 4, false)
│ └─ ← [Revert] CollectionMappingAlreadySet()
├─ [0] VM::prank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [24900] Starklane::setL1L2CollectionMapping(SHA-256: [0x0000000000000000000000000000000000000002], 4, true)
│ ├─ emit L1L2CollectionMappingUpdated(colllectionL1: SHA-256: [0x0000000000000000000000000000000000000002], collectionL2: 4)
│ └─ ← [Stop]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.35ms (152.20µs CPU time)
​
Ran 1 test suite in 844.75ms (1.35ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Impact

  • Legitimate mappings can be overwritten, leading to potential data inconsistencies.

  • Critical operations relying on stable mappings may be disrupted.

Tools Used

  • Manual Review

  • Foundry

Recommendations

  • Implement additional checks or confirmations when force is used to ensure intentional overwriting.

  • Consider additional access controls or multi-signature requirements for operations involving forced overwrites.

Updates

Lead Judging Commences

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

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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