NonFungibleTokenDO Manual Review Findings

NFT-01M: Insecure Batch Transfer Operations

Type	Severity	Location
Input Sanitization		NonFungibleTokenDO.sol:L194-L211, L222-L242

Description:

The referenced batch transfer operations are currently not utilized by an active implementation of the system, however, they are still accessible via direct invocation from a data point's administrator.

They are presently insecurely implemented as they do not mandate that the input token IDs are unique, permitting the same token ID's owners data entry to be overridden multiple times even though more than one transfers were meant to be performed.

Impact:

The current batch transfer implementations permit a single token ID to be transferred multiple times artificially reducing and inflating the balance of two parties, potentially causing NFTs to be come inaccessible due to a subtraction underflow.

Example:

Copy
185/**
186 * @notice Operation used to transfer batch of tokens
187 * @param dp DataPoint identifier
188 * @param from The account address to transfer the tokens from
189 * @param to The account address to transfer the tokens to
190 * @param tokenIds The tokenIds of tokens to transfer
191 * @dev If the balance is insufficient, the operation will revert with tokenId 0 and not the
192 *     actual tokenId that caused the error, to save gas
193 */
194function _batchTransferFrom(DataPoint dp, address from, address to, uint256[] memory tokenIds) internal {
195    bytes32 diidFrom = _diid(dp, from);
196    bytes32 diidTo = _diid(dp, to);
197    DiidData storage diiddFrom = _diidData(dp, diidFrom);
198    DiidData storage diiddTo = _diidData(dp, diidTo);
199
200    uint256 tokenIdsLength = tokenIds.length;
201
202    _decreaseBalance(diiddFrom, dp, diidFrom, tokenIdsLength, 0);
203    _increaseBalance(diiddTo, dp, diidTo, tokenIdsLength);
204
205    for (uint256 i = 0; i < tokenIdsLength; i++) {
206        uint256 tid = tokenIds[i];
207
208        DpData storage dpd = _dpData(dp);
209        dpd.owners[tid] = to;
210    }
211}

Recommendation:

We advise the code to ensure the token IDs are unique, for example by ensuring they have been specified in a strictly ascending or descending order.

Alleviation (c6b23c23d8bcd8cce85049ad959cbd711a37126b):

A NonFungibleTokenDO::_validateTokenOrder function was introduced that ensures that the NFT IDs involved in a batch transfer are defined in a strictly ascending order, preventing duplicate token IDs from being defined in a batch transfer and thus alleviating this exhibit in full.