Summary

The function _withdrawFromEscrow does not fully adhere to the Checks-Effects-Interactions (CEI) pattern, which could potentially expose the contract to reentrancy attacks. The function performs an external call to safeTransferFrom which triggers a callback to an arbitrary address before the internal state is updated, though there are some mitigating factors in place.

Vulnerability Details

The _withdrawFromEscrow function interacts with external contracts through safeTransferFrom and potentially exposes the contract to reentrancy attacks due to the order of operations. Here’s a breakdown:

• Checks: The function checks whether the token is escrowed using _isEscrowed.

• Effects: The internal state _escrow is updated after the external call.

• Interactions: The function interacts with external contracts via safeTransferFrom.

Reentrancy Risks:

• ERC721 Collection: safeTransferFrom for ERC721 tokens calls out to external function onERC721Received in the receivers contract (if the receiver is a contract), which could trigger reentrancy attacks if the external contract is malicious.

• ERC1155 Collection: While ERC1155’s safeTransferFrom also interacts with external contracts, it is currently not implemented, but it may pose similar risks when implemented.

Impact

Reentrancy vulnerabilities can lead to:

• Loss of Tokens: Malicious contracts could exploit the vulnerability to withdraw tokens multiple times.

• Inconsistent State: The contract’s internal state might become inconsistent if an external call is exploited.

Tools Used

Manual Code Review: Identified CEI pattern violations and potential reentrancy risks.

Recommendations

Move State Updates Before External Calls: Update the internal state _escrow before calling safeTransferFrom. This will prevent reentrancy attacks as the contract’s state is updated before the external call is made.