ArkProject: NFT Bridge

In the https://github.com/Cyfrin/2024-07-ark-project/apps/blockchain/ethereum/src/Escrow.sol
The StarklaneEscrow contract is designed to handle the escrow of ERC721 and ERC1155 tokens. However, it contains a significant vulnerability related to the handling of ERC1155 token supplies. This report details the nature of the vulnerability, its potential impact, and recommendations for mitigation.

Vulnerability Details
Issue: The contract assumes that each ERC1155 token deposited into escrow has a supply of exactly one but does not enforce or verify this assumption. This can lead to incorrect behavior when handling tokens with a different supply.

Location: The vulnerability is found in the _depositIntoEscrow and _withdrawFromEscrow functions.

Code Snippet
if (collectionType == CollectionType.ERC1155) {
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
}
The code snippet above shows that the contract unconditionally transfers a single unit of the ERC1155 token, assuming the supply is one. However, ERC1155 tokens can have varying supplies, and this assumption can lead to incorrect accounting and logic errors.

Impact
Incorrect Token Accounting: If a token with a supply greater than one is deposited, the contract will not account for the additional units correctly. This can lead to over or under-escrowed tokens, resulting in potential loss or incorrect state management.
Potential Exploits: Malicious users can exploit this assumption to manipulate the escrow process. For instance, a user could deposit multiple units of an ERC1155 token, but the contract would only track one unit, potentially allowing the user to withdraw more than they should be entitled to.
Inconsistent State: The contract state could become inconsistent with the actual token balances, leading to issues in token transfers, withdrawals, and overall contract functionality.

Proof of Concept
Consider an ERC1155 token with a supply of 5 units. A user deposits all 5 units into the escrow

erc1155.safeTransferFrom(msg.sender, address(this), id, 5, "");

The contract only tracks a single unit

_escrow[collection][id] = msg.sender;

When the user withdraws the token, the contract assumes only one unit was deposited and allows the user to withdraw all units

Recommendations
Supply Verification: Implement checks to verify that the ERC1155 token supply is exactly one before accepting the deposit. This can be done by querying the token's balance before and after the transfer and ensuring only one unit is transferred.
Update Documentation: Clearly document the assumptions and requirements for ERC1155 tokens to ensure developers and users understand the constraints.
Additional Testing: Perform extensive testing with various ERC1155 token supplies to ensure the contract handles all scenarios correctly.

Mitigation Example
Here is an example of how to implement a supply verification check in the _depositIntoEscrow function:

if (collectionType == CollectionType.ERC1155) {
uint256 preBalance = IERC1155(collection).balanceOf(address(this), id);
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
uint256 postBalance = IERC1155(collection).balanceOf(address(this), id);
require(postBalance == preBalance + 1, "ERC1155 token supply must be exactly one");
}
This ensures that the contract only accepts ERC1155 tokens with a supply of one and prevents incorrect accounting.