disabling max lock does not correctly reset maxLockIdToIndex leading to some tokenIDs permanently stuck
Summary
The disable_max_lock is meant to reset the maxLockIdToIndex for an NFT to zero, so that it will not be max locked on any interaction going forward.
But for some tokens, this maxLockIdToIndex is never reset causing critical problems.
Vulnerability Detail
Once an NFT is enabled for max lock it gets a non-zero index stored against the tokenID in maxLockIdToIndex, which behaves as an identifier of if the nft has been max locked.
This is disable_max_lock function :
function disable_max_lock(uint _tokenId) external {
assert(_isApprovedOrOwner(msg.sender, _tokenId));
require(maxLockIdToIndex[_tokenId] != 0,"disabled");
uint index = maxLockIdToIndex[_tokenId] - 1;
maxLockIdToIndex[_tokenId] = 0;
// Move the last element into the place to delete
max_locked_nfts[index] = max_locked_nfts[max_locked_nfts.length - 1];
// update the index
maxLockIdToIndex[max_locked_nfts[index]] = index + 1;
// Remove the last element
max_locked_nfts.pop();
}
This logic is swapping the relevant tokenID and replacing it with the last element in the max_locked_nfts array, then popping the last element.
This handles normal caes properly by resetting the disabled tokenID's maxLockIdToIndex to 0, and that is important so that the tokenID doesn't get re-locked automatically hereafter (see max_lock logic here)
But if the last element from the max_locked_nfts array is being disabled, the current logic does not handle that case correctly.
Lets go through an example :
Assume there are 3 nfts in the max_locked_nfts array at array index 0,1,2
A user who owns the last element calls disable_max_lock
maxLockIDINdex will be = 3, index = 2, and now sets maxLockToIndex[tokenID] = 0
max_locked_nfts[2] = max_locked_nfts[max_locked_nfts.length - 1] , but these elements are already the same coz the tokenID was the last element in the array
maxLockIdToIndex[max_locked_nfts[2]] = index + 1; which again sets the maxLockIdIndex of tokenID to 3
Boom, now we have a non-zero maxLockIdToIndex for a tokenID which has actually been popped from the max_locked_nfts and the user wanted to disable its max lock feature.
This will cause many critical problems because max_Lock logic now considers it as still max lock enabled due to maxLockIdToIndex being non-zero.
Withdrawl is permanently bricked => withdraw calls isApprovedOrOwner => max_lock which increases the unlock time of the tokenID in lockedBalance state. But this will cause revert here because withdraw function requires the lock to have been expired.
max_lock is a public function so anyone can now indefinitely extend the lock even though the user wanted to disable it and maybe unlock the position.
If user tries to call disable again for the same tokenID, it will try to read the element in the array using tokenID's maxLockIdToIndex but the array will either hold nothing at that index or will hold a different tokenID, thus corrputing the storage further.
Impact
Disable_max_lock does not correctly reset the maxLockIdToIndex for some tokens, which allows anyone to max_lock the nft even after the user tried to disable it. This will prevent users from accessing their locked tokens.
High severtiy because this can happen under normal operations without any external conditions, and can happen repeatedly for many users causing critical problems.
Refactor the logic to accomodate the special case of when the array's last element is being removed, similar to how its done in removeTokenFromOwnerList
Chinmay
High
disabling max lock does not correctly reset maxLockIdToIndex leading to some tokenIDs permanently stuck
Summary
The disable_max_lock is meant to reset the maxLockIdToIndex for an NFT to zero, so that it will not be max locked on any interaction going forward.
But for some tokens, this maxLockIdToIndex is never reset causing critical problems.
Vulnerability Detail
Once an NFT is enabled for max lock it gets a non-zero index stored against the tokenID in maxLockIdToIndex, which behaves as an identifier of if the nft has been max locked. This is disable_max_lock function :
This logic is swapping the relevant tokenID and replacing it with the last element in the max_locked_nfts array, then popping the last element.
This handles normal caes properly by resetting the disabled tokenID's maxLockIdToIndex to 0, and that is important so that the tokenID doesn't get re-locked automatically hereafter (see max_lock logic here)
But if the last element from the max_locked_nfts array is being disabled, the current logic does not handle that case correctly.
Lets go through an example :
Boom, now we have a non-zero maxLockIdToIndex for a tokenID which has actually been popped from the max_locked_nfts and the user wanted to disable its max lock feature.
This will cause many critical problems because max_Lock logic now considers it as still max lock enabled due to maxLockIdToIndex being non-zero.
Impact
Disable_max_lock does not correctly reset the maxLockIdToIndex for some tokens, which allows anyone to max_lock the nft even after the user tried to disable it. This will prevent users from accessing their locked tokens.
High severtiy because this can happen under normal operations without any external conditions, and can happen repeatedly for many users causing critical problems.
Code Snippet
https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L893-L908
Tool used
Manual Review
Recommendation
Refactor the logic to accomodate the special case of when the array's last element is being removed, similar to how its done in removeTokenFromOwnerList
Duplicate of #257