retrieveTokenStatus Logic Not Matching Expectation

[mpeyfuss]

Summary

The logic to retrieve a token delegation status does not make sense in the current implementation as it strictly enforces a delegation to be made for a single token on the collection. This means that if I delegate for all tokens on a collection, this view function will return false for a specific token. In my opinion, this is broken logic and does not match the interface description.

Relevant Source Code

nftdelegation/smart_contracts/DelegationManagement.sol

Line 619 in 50e3d17

for (uint256 i = 0; i < globalDelegationHashes[hash].length; ) {

for (uint256 i = 0; i < globalDelegationHashes[hash].length; ) {
    if ((globalDelegationHashes[hash][i].allTokens == false) && (globalDelegationHashes[hash][i].tokens == _tokenId)) {
        status = true;
        break;
    } else {
        status = false;
    }

    unchecked {
        ++i;
    }
}
return status;

You can see that in the first if statement that the check requires that the property allTokens must be false. This means that all delegations currently coming from https://seize.io/delegation/delegation-center will cause this retreieveTokenStatus to return false as the frontend supplies a value of true for _allTokens in the call to registerDelegationAddress.

In addition, the expiration date is not checked in this function, which could be damaging based on how it is being used either off-chain or on-chain.

Potential Solution

for (uint256 i = 0; i < globalDelegationHashes[hash].length; ) {
    if ((globalDelegationHashes[hash][i].allTokens || globalDelegationHashes[hash][i].tokens == _tokenId) && globalDelegationHashes[hash][i].expiryDate > block.timestamp) {
        status = true;
        break;
    } else {
        status = false;
    }

    unchecked {
        ++i;
    }
}
return status;

This fix simply checks that the delegation hash for all tokens OR the specific token id passed into the function AND ensures that the expiry time is greater than the current block timestamp. In my opinion, this matches the expected use case for a function like this and doesn't require other function calls to check allTokens or expiry time.

[a2rocket]

The retrieveTokenStatus() function was designed as an additional function to check directly the delegation status given a token id. There are other retrieve functions which can help you check delegations either on all tokens or on a specific token as well as expiry date.

For example the retrieveDelegatorsTokensIDsandExpiredDates(...) function returns arrays of all delegators, tokenids, if they delegated on allTokens or on a specific token id as well as the expiry dates of the delegations. A developer can get those data and treat them as he/she wants :)

[mpeyfuss]

The retrieveTokenStatus() function was designed as an additional function to check directly the delegation status given a token id. There are other retrieve functions which can help you check delegations either on all tokens or on a specific token as well as expiry date.

This comment makes it seem like the purpose of the function is to check the current delegation status. If so, this MUST check all tokens and expiry date in my opinion. Otherwise, you are just returning invalid data that can be incorrectly used by third party devs.