6529-Collections / nftdelegation

Delegation contract
MIT License
16 stars 7 forks source link

Retrieve functions do not check `expiryTime` #50

Open mpeyfuss opened 10 months ago

mpeyfuss commented 10 months ago

Problem Statement

Most, if not all, retrieve functions do not check the expiry time of a delegation. You can see in this example function (retrieveGlobalStatusOfDelegation) that there is no check of expiry time. A global delegation may be expired, but this function will return back true as the length of the array is greater than 0.

https://github.com/6529-Collections/nftdelegation/blob/50e3d171077b72cd043562cd0801fc188934bc94/smart_contracts/DelegationManagement.sol#L604

This can lead to confusion for developers trying to use this delegation registry, especially if they aren't fluent in Solidity. Perhaps this stems from my expectations based on the description of the retrieve functions, but in my opinion, the code should make it as simple as possible and not rely on logic to be implemented elsewhere. This registry should act as the source of truth any time I call it and not rely on logic to be implemented elsewhere. If I were going to check these functions in another contract, I want the truth returned to me so I can use it for my own purposes.

mpeyfuss commented 10 months ago

Something else that is likely important is that retrieveDelegators does not check expiry times which could allow for expired sub-delegators to act after their time has expired.

a2rocket commented 10 months ago

Taking into account the expiry date of a delegation clearly depends on the developer who will integrate the smart contract. For this purpose we have developed a specific retrieve function namely, retrieveActiveDelegators(...) that takes into consideration a specific epoch time.

By executing this function you can retrieve only the active delegators for a specific delegation address based on the expiry date.

mpeyfuss commented 10 months ago

If the contract defines an expiry time, shouldn't that be enforced everywhere? Seems like an easy way to lead to improper integration with third party apps. At least naming the functions something like retrieveAllHistoricalDelegates would give a clue to devs on what is returned.

Additionally, better documentation + natspec comments on these considerations would be incredibly helpful. Right now, devs just have to guess at the purpose of a function based on one comment in the source code and aren't aware of any pitfalls, such as not checking expiry time.