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

Unbounded Loop and Lack of Input Validation in Starklane(Bridge) Bridge Contract

juggernaut63

Summary

The Starklane(Bridge) Bridge Contract contains two critical vulnerabilities: an unbounded loop in the getWhiteListedCollections function and insufficient input validation in the setL1L2CollectionMapping function. These issues can lead to denial of service, functional disruptions, and potential asset loss.

This bug is caused by the lack of control access on _withdrawFromEscrow which I explained in the previous report: https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Escrow.sol#L63-L89

Vulnerability Details

  1. Unbounded Loop in getWhiteListedCollections:

    • The getWhiteListedCollections function iterates over the entire _collections array without any upper limit.

    • This can lead to excessive gas consumption, especially when the array grows large due to multiple collections being added.

  2. Lack of Input Validation in setL1L2CollectionMapping:

    • The setL1L2CollectionMapping function does not validate the input addresses.

    • Invalid addresses can be set, disrupting the contract's functionality.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "../src/Bridge.sol";
import "../src/token/ERC721Bridgeable.sol";
​
contract ExploitTests is Test {
Starklane bridge;
ERC721Bridgeable maliciousCollection;
​
function setUp() public {
bridge = new Starklane();
maliciousCollection = new ERC721Bridgeable();
// Initialize the bridge contract
bytes memory data = abi.encode(address(this), address(0x123), uint256(0x456), uint256(0x789));
bridge.initialize(data);
// Disable whitelist
bridge.enableWhiteList(false);
// Enable the bridge
bridge.enableBridge(true);
}
​
function testUnboundedLoop() public {
// Add many collections to the whitelist (reduced number to avoid excessive gas consumption)
for (uint256 i = 0; i < 100; i++) {
address newCollection = address(new ERC721Bridgeable());
bridge.whiteList(newCollection, true);
}
​
// Call getWhiteListedCollections and expect it to consume excessive gas
address[] memory whiteListedCollections = bridge.getWhiteListedCollections();
assertEq(whiteListedCollections.length, 100);
}
​
function testLackOfInputValidation() public {
// Set invalid mappings
bridge.setL1L2CollectionMapping(address(0), snaddress.wrap(uint256(uint160(address(0x456)))), true);
bridge.setL1L2CollectionMapping(address(this), snaddress.wrap(uint256(0)), true);
​
// Check if the mappings are set
// This should disrupt the functionality of the bridge
// Further tests can be added to check the impact of these invalid mappings
}
}

forge test --match-path test/ExploitTests.t.sol
[⠊] Compiling...
No files changed, compilation skipped

Ran 2 tests for test/ExploitTests.t.sol:ExploitTests
[PASS] testLackOfInputValidation() (gas: 61823)
[PASS] testUnboundedLoop() (gas: 181264591)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 91.31ms (85.11ms CPU time)

Ran 1 test suite in 102.98ms (91.31ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)

Impact

  • Unbounded Loop: Denial of Service, Operational Disruption

  • Lack of Input Validation: Functional Disruption, Security Risks

Tools Used

  • Manual review

  • Foundry

Recommendations

  • Implement a maximum limit on the number of collections that can be added to the whitelist.

uint256 constant MAX_COLLECTIONS = 100; // Example limit
​
function whiteList(address collection, bool enable) external onlyOwner {
require(_collections.length < MAX_COLLECTIONS, "Too many collections");
_whiteListCollection(collection, enable);
emit CollectionWhiteListUpdated(collection, enable);
}

  • Make sure the address is valid.

function setL1L2CollectionMapping(address collectionL1, snaddress collectionL2, bool force) external onlyOwner {
require(collectionL1 != address(0), "Invalid L1 collection address");
require(snaddress.unwrap(collectionL2) != 0, "Invalid L2 collection address");
_setL1L2AddressMapping(collectionL1, collectionL2, force);
emit L1L2CollectionMappingUpdated(collectionL1, snaddress.unwrap(collectionL2));
}

  • Access Control: Ensure that sensitive functions are protected with appropriate access control mechanisms.

  • Gas Optimization: Regularly review and optimize functions that may consume excessive gas.

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.