checkERC1155BeforePull Function in DelegateTokenTransferHelpers

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenTransferHelpers.sol#L61

Vulnerability details

Description

The checkERC1155BeforePull function in Contract XYZ has a potential issue where it reverts if pullAmount is equal to 0, which might not always be the desired behavior for ERC1155 tokens.

Issue Details

Context

In DelegateTokenTransferHelpers.sol, there is a function called checkERC1155BeforePull designed to handle ERC1155 token transfers. It checks whether the pullAmount is zero and reverts if it is.

Problem Description

The problem arises because ERC1155 tokens can represent multiple units of a single asset, and there are valid scenarios where a caller might want to pull all units of an ERC1155 token (i.e., pullAmount is 0). However, the current implementation prohibits this action by reverting when pullAmount is 0.

Explanation

ERC1155 tokens are versatile and can represent a variety of assets, including collections of items or units. In some cases, it might be legitimate for a user to want to transfer all units of an ERC1155 token to another address. However, the checkERC1155BeforePull function in Contract XYZ does not allow this, as it strictly requires a non-zero pullAmount.

The checkERC1155BeforePull function will revert if pullAmount is equal to 0. This is because the function is explicitly checking if the pullAmount is 0 for ERC1155 tokens.

The reason for this is that ERC1155 tokens can represent multiple units of a single asset, such as a set of collectible cards. If the pullAmount is 0, it would mean that the caller is trying to pull all of the units of the ERC1155 token, which is not possible.

However, it is possible that the caller may want to pull a specific unit of an ERC1155 token, in which case the pullAmount would be non-zero. In this case, the checkERC1155BeforePull function would not revert.

Impact

The current behavior of the checkERC1155BeforePull function may cause issues for users who intend to transfer all units of an ERC1155 token in a single transaction. The suggested solution will improve the flexibility of the contract and align it better with the usage patterns of ERC1155 tokens.

Suggested Solution

To resolve this issue, the contract should be updated to allow for a pullAmount of 0 in the case of ERC1155 tokens. Here's a suggested modification to the checkERC1155BeforePull function:

 function checkERC1155BeforePull(Structs.Uint256 storage erc1155Pulled, uint256 pullAmount) internal {
     if (erc1155Pulled.flag == ERC1155_NOT_PULLED) {
         erc1155Pulled.flag = ERC1155_PULLED;
     } else {
         revert Errors.ERC1155Pulled();
     }
 }

This modification removes the explicit check for a non-zero pullAmount and focuses solely on the flag associated with ERC1155 token transfers. By doing this, the contract will allow for both non-zero and zero pullAmount values when dealing with ERC1155 tokens.

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

Imaginary issue