ArkProject: NFT Bridge

Summary

The Escrow.sol contract, which manages the escrowing of tokens, currently lacks non-reentrancy guards and does not fully implement the Checks-Effects-Interactions (CEI) pattern. This combination of vulnerabilities exposes the contract to reentrancy attacks, where an external call could lead to unintended state changes or asset losses, compounded by the failure to perform state changes (effects) before interacting with external contracts.

Vulnerability Details

Lack of Non-Reentrancy Guards

The absence of non-reentrancy guards in critical functions such as _depositIntoEscrow and _withdrawFromEscrow makes the contract susceptible to reentrancy attacks, where an attacker could exploit the lack of safeguards to perform multiple malicious operations before the contract's state is updated.In addition to this, the current implementation of the Escrow.sol contract makes external calls to transfer tokens before updating the contract's state, violating the CEI pattern.

Affected Code Block: _depositIntoEscrow Function

Affected Code Block: _withdrawFromEscrow Function

Impact

1. Reentrancy Attack :

   • Attack Scenario: An attacker could re-enter the _withdrawFromEscrow function through a malicious contract, causing the function to be called multiple times before the escrow state is updated. This could lead to multiple withdrawals of the same token, as the state change that marks the token as withdrawn (_escrow[collection][id] = address(0x0)) happens after the token transfer.

   • Impact: This could result in unauthorized and repeated withdrawals of NFTs or ERC1155 tokens, leading to significant financial losses for users and the project.

2. Multiple Deposits/Withdrawals:

   • Attack Scenario: During the _depositIntoEscrow or _withdrawFromEscrow operations, the attacker could exploit the lack of reentrancy protection to repeatedly deposit or withdraw tokens before the contract updates its internal state, leading to an inconsistent state or duplicate actions.

   • Impact: This can lead to unauthorized multiple deposits or withdrawals of tokens, disrupting the escrow logic and leading to asset loss or theft.

3. Manipulation of Contract Behavior:

   • Attack Scenario: The attacker could trigger reentrant calls that manipulate the contract’s state in ways that are unintended by the developers. For instance, calling _withdrawFromEscrow repeatedly before the escrow mapping is updated can lead to inconsistencies in token ownership records.

   • Impact: This manipulation could disrupt the contract's intended behavior, leading to failures in managing token escrows and reducing the reliability and trust in the contract.

Potential Financial Impact

1. Unauthorized Withdrawals and Asset Theft:

   • Risk: Exploiting the reentrancy vulnerabilities could enable attackers to withdraw tokens multiple times or unauthorizedly, leading to significant asset theft.

   • Financial Impact: For valuable NFTs or high-volume ERC1155 tokens, this could result in millions of dollars in losses, severely damaging Ark Project's reputation.

2. Systemic Contract Failures:

   • Risk: Repeated attacks could lead to systemic failures in the escrow system, causing widespread token mismanagement and transaction failures.

   • Financial Impact: The loss of user trust and the cost of rectifying such systemic issues could be substantial, potentially leading to legal repercussions and financial penalties.

Tools Used

• Manual Code Review: Identified missing non-reentrancy protections and CEI pattern violations.

• Static Analysis Tools: Reviewed contract interactions and state changes for CEI compliance.

Recommendations

1. Implement Non-Reentrancy Guards:
   Apply the nonReentrant modifier to all functions interacting with external contracts or performing state changes:

1. Adhere to the CEI Pattern:
   Ensure that all state changes are performed before making external calls to prevent reentrancy attacks:

   function _withdrawFromEscrow(

   CollectionType collectionType,

   address collection,

   address to,

   uint256 id

   )

   internal

   returns (bool)

   {

   if (!_isEscrowed(collection, id)) {

   return false;

   }

   ​

   // Update state before making external call

   _escrow[collection][id] = address(0x0);

   ​

   address from = address(this);

   ​

   if (collectionType == CollectionType.ERC721) {

   IERC721(collection).safeTransferFrom(from, to, id);

   } else {

   IERC1155(collection).safeTransferFrom(from, to, id, 1, "");

   }

   ​

   return true;

   }

2. Review Internal Functions:
   Ensure that any internal functions interacting with external contracts also incorporate both non-reentrancy safeguards and the CEI pattern to mitigate the combined risks.