ArkProject: NFT Bridge

Summary

A potential issue was identified in the depositTokens function of the Starklane contract, where there is no check to ensure that the ids array is non-empty. This could lead to unintended behavior, such as gas wastage or incorrect state recording.

Vulnerability Details

The depositTokens function accepts an array of token IDs (ids) as an argument. However, there is no validation to ensure that this array is not empty. If an empty array is passed, the function continues to execute, despite the fact that the operation of depositing tokens is meaningless without any tokens specified.

Key observations:

1. The ids array is used in various parts of the function:

   • Passed to TokenUtil.erc721Metadata.

   • Used in _depositIntoEscrow.

   • Assigned to req.tokenIds.

   • Its length is checked to determine if the payload is too long.

2. Without a non-empty ids array, the function can perform unnecessary operations, leading to wasted gas and possibly incorrect contract state.

Impact

The impact of this issue is classified as Low to Medium severity. Although it doesn't introduce a critical vulnerability, it can lead to:

1. Gas Wastage: The function performs operations even when there are no tokens to deposit, leading to unnecessary gas consumption.

2. Incorrect State: An empty deposit might be recorded, which does not make sense in the context of an NFT bridge and could lead to unexpected behavior.

3. Potential Issues on L2: If the Layer 2 contract doesn't handle empty token arrays properly, it could cause unforeseen issues.

Tools Used

Manual Review

Recommendations

To handle the issue of an empty ids array, consider using an early return pattern. This approach checks if the ids array is empty and exits the function early without executing the rest of the logic. Here's how you can do it: