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

Unrestricted Access to deployERC721Bridgeable and deployERC1155Bridgeable

Rareone

In the contract https://github.com/Cyfrin/2024-07-ark-project/apps/blockchain/ethereum/src/token/Deployer.sol

The functions deployERC721Bridgeable and deployERC1155Bridgeable in the Deployer library are publicly accessible. This means any address can call these functions to deploy new instances of ERC721 or ERC1155 contracts.

Impact:

Unauthorized users can deploy new contracts, leading to unexpected contract creations.
This can result in network congestion, unexpected gas costs, and potential misuse of resources.
Proof of Concept:
The following tests were executed to confirm the vulnerability:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Test} from "forge-std/Test.sol";
import {Deployer} from "../src/token/Deployer.sol";

contract UnrestrictedAccessTest is Test {
address owner;
address nonOwner;

function setUp() public {
owner = address(this); // The test contract itself is the owner
nonOwner = address(0x1234); // Some other address
}
​
function testDeployERC721BridgeableUnrestrictedAccess() public {
vm.prank(nonOwner); // Impersonate the non-owner address
address proxy = Deployer.deployERC721Bridgeable("Test721", "TST721");
require(proxy != address(0), "Deployment should not be allowed for non-owner");
}
​
function testDeployERC1155BridgeableUnrestrictedAccess() public {
vm.prank(nonOwner); // Impersonate the non-owner address
address proxy = Deployer.deployERC1155Bridgeable("http://test.uri/");
require(proxy != address(0), "Deployment should not be allowed for non-owner");
}

}

Results:

Ran 2 tests for test/Exploit.t.sol:UnrestrictedAccessTest
[PASS] testDeployERC1155BridgeableUnrestrictedAccess() (gas: 2170608)
[PASS] testDeployERC721BridgeableUnrestrictedAccess() (gas: 1936736)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 8.68ms (7.96ms CPU time)

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

The tests passed, confirming that non-owner addresses could deploy new instances of ERC721 and ERC1155 contracts.

Mitigation:
To mitigate this vulnerability, add access control to the deployERC721Bridgeable and deployERC1155Bridgeable functions. Here is how you can do it using OpenZeppelin's Ownable contract:
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import "./ERC721Bridgeable.sol";
import "./ERC1155Bridgeable.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

/**
@title Collection contract deployer.
*/
contract Deployer is Ownable {

/**
@notice Deploys a UUPS proxied ERC721 contract.
​
@param name Descriptive name for the token collection.
@param symbol Abbreviated name for the token collection.
​
@return Address of the proxy.
*/
function deployERC721Bridgeable(
string memory name,
string memory symbol
)
public
onlyOwner
returns (address)
{
address impl = address(new ERC721Bridgeable());
bytes memory dataInit = abi.encodeWithSelector(
ERC721Bridgeable.initialize.selector,
abi.encode(name, symbol)
);
​
return address(new ERC1967Proxy(impl, dataInit));
}
​
/**
@notice Deploys a UUPS proxied ERC1155 contract.
​
@param uri URI with token id placeholder.
​
@return Address of the proxy.
*/
function deployERC1155Bridgeable(
string memory uri
)
public
onlyOwner
returns (address)
{
address impl = address(new ERC1155Bridgeable());
bytes memory dataInit = abi.encodeWithSelector(
ERC1155Bridgeable.initialize.selector,
abi.encode(uri)
);
​
return address(new ERC1967Proxy(impl, dataInit));
}

}

Explanation
Ownable Contract: The Deployer contract now inherits from OpenZeppelin's Ownable contract.
Access Control: The onlyOwner modifier is added to deployERC721Bridgeable and deployERC1155Bridgeable functions to restrict access to only the contract owner.
Conclusion
By adding access control, you ensure that only authorized users can deploy new instances of ERC721 and ERC1155 contracts, preventing unauthorized and potentially malicious deployments.

Tools used: Foundry

Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
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.