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

Low Findings Bundle

adamszymanski

Summary

  1. Incorrect if statement placement

  2. CEI pattern not satisfied

  3. Misuse of the assert() function

  4. Floating Solidity compiler version declaration

Vulnerability Details

Incorrect if Statement Placement

The if statement should be placed before line 95 to prevent the unnecessary execution of detectInterface().

Bridge.sol (lines 95–102)

// ✔ Correct if statement placement
// if (!_isWhiteListed(collectionL1)) {
// revert NotWhiteListedError(); //
// }
​
CollectionType ctype = TokenUtil.detectInterface(collectionL1);
​
if (ctype == CollectionType.ERC1155) {
revert NotSupportedYetError();
}
​
if (!_isWhiteListed(collectionL1)) {
revert NotWhiteListedError();
// The if statement responsible for _isWhiteListed(collectionL1) should be placed before ctype declaration
}

CEI Pattern Not Satisfied & Misuse of assert()

The assert() function should be used exclusively for checking internal errors and invariants in the contract's code.

The CEI (Conditions, Effects, Interactions) pattern is not satisfied because the effect—updating the escrow mapping—should be placed before interactions like safeTransferFrom and transferFrom to ensure that state changes are made before executing external calls, thus maintaining consistency and reducing potential issues.

Escrow.sol (lines 26–51)

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
// ✔ Correct Effect placement
//_escrow[collection][id] = msg.sender;
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(msg.sender, address(this), id);
} else {
// TODO: check the supply is exactly one.
// (this is the low level call to verify if a contract has some function).
// (but it's better to check with supported interfaces? It's 2 calls instead
// of one where we control the fail.)
//(bool success, bytes memory data) = contractAddress.call("");
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
}
​
_escrow[collection][id] = msg.sender;
// Update of escrow mapping should be placed before if/else block in which Interaction happens
}
}

Floating Solidity Compiler Version Declaration

The Solidity compiler version declaration across all .sol files is floating: pragma solidity ^0.8.0;

It should be declared as a static version, e.g.,: pragma solidity 0.8.26;

Using a static Solidity version is vital to prevent unexpected vulnerabilities from future compiler updates. It ensures consistent contract behavior, secure code execution, and reliable audits, reducing the risk of security breaches.


Impact

The issues identified in the audit underscore the importance of adhering to secure coding standards in smart contract development. Inconsistent logic flow, improper error handling, and potential exposure to future vulnerabilities can compromise the contract's reliability and security. Ensuring that code is structured correctly, patterns are followed, and versioning is tightly controlled is essential for maintaining the integrity, predictability, and safety of the contract in production. Failing to address these areas could lead to unpredictable behavior, security breaches, and increased maintenance costs.

Tools Used

Manual auditing.


Recommendations

Apply the recommendations outlined in each Vulnerability Details paragraph.

Updates

Lead Judging Commences

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

Support

FAQs

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