Aymen0909 - Users will not be able to reclaim voting tokens after a shutdown is cancelled

[sherlock-admin2]

Aymen0909

High

Users will not be able to reclaim voting tokens after a shutdown is cancelled

Summary

When a shutdown vote gets cancelled by a CollectionShutdown::cancel call, the _collectionParams struct is deleted which will prevent users who did previously vote from reclaiming their tokens through CollectionShutdown::reclaimVote because the params.shutdownVotes decrement will underflow and revert the call, this will cause the loss of all users funds used for that vote (which will remain stuck in the contract).

Root Cause

• When an ongoing shutdown vote gets cancelled through CollectionShutdown::cancel, the storage struct _collectionParams get deleted (see CollectionShutdown::403) meaning all votes shutdownVotes are reset to 0.
• When voters try to reclaim their voting tokens with CollectionShutdown::reclaimVote, the function will revert because params.shutdownVotes -= uint96(userVotes) (see CollectionShutdown::369) will always underflow at this point as shutdownVotes == 0, thus the reclaim call will always revert.

Internal pre-conditions

1. Shutdown vote must be ongoing (did not reach quorum yet) and some users did transfer their tokens in the contract to vote.
2. The total supply of the token does increase again above shutdown threshold which allow anyone to trigger the cancel of the shutdown.

External pre-conditions

None

Attack Path

1. A shutdown vote for a collection is ongoing.
2. Users: Alice, Bob, Charles vote in favor of the shutdown.
3. Vote doesn't reach the quorum threshold yet, and in the mean time the tokens total supply increases again above the shutdown threshold.
4. Another user (could be anyone) named Carol notices that and invokes CollectionShutdown::cancel to cancel the vote, this does delete all the collection shutdown data including shutdownVotes
5. Now that shutdown got cancelled, Alice, Bob, Charles will try to get their tokens back by calling CollectionShutdown::reclaimVote but it will be impossible for them as the function will revert because of the underflow error when subtracting from shutdownVotes
6. Alice, Bob, Charles lose their tokens which remain stuck in the contract.

Impact

In case of a vote cancelling ALL voters will be unable to reclaim their tokens used for voting, all tokens will be lost and remain stuck in the contract.

PoC

The CollectionShutdown::cancel function does delete the whole collection shutdown struct as shown below:

function cancel(address _collection) public whenNotPaused {
    // Ensure that the vote count has reached quorum
    CollectionShutdownParams memory params = _collectionParams[_collection];
    if (!params.canExecute) revert ShutdownNotReachedQuorum();

    // Check if the total supply has surpassed an amount of the initial required
    // total supply. This would indicate that a collection has grown since the
    // initial shutdown was triggered and could result in an unsuspected liquidation.
    if (params.collectionToken.totalSupply() <= MAX_SHUTDOWN_TOKENS * 10 ** locker.collectionToken(_collection).denomination()) {
        revert InsufficientTotalSupplyToCancel();
    }

    // Remove our execution flag
    delete _collectionParams[_collection];
    emit CollectionShutdownCancelled(_collection);
}

Thus in case of a cancel, when voters try to reclaim using CollectionShutdown::reclaimVote, the function will revert from underflow when decreasing from shutdownVotes

function reclaimVote(address _collection) public whenNotPaused {
    // If the quorum has passed, then we can no longer reclaim as we are pending
    // an execution.
    CollectionShutdownParams storage params = _collectionParams[_collection];
    if (params.canExecute) revert ShutdownQuorumHasPassed();

    // Get the amount of votes that the user has cast for this collection
    uint userVotes = shutdownVoters[_collection][msg.sender];

    // If the user has not cast a vote, then we can revert early
    if (userVotes == 0) revert NoVotesPlacedYet();

    // We delete the votes that the user has attributed to the collection
    params.shutdownVotes -= uint96(userVotes);
    delete shutdownVoters[_collection][msg.sender];

    // We can now return their tokens
    params.collectionToken.transfer(msg.sender, userVotes);

    // Notify our stalkers that a vote has been reclaimed
    emit CollectionShutdownVoteReclaim(_collection, msg.sender, userVotes);
}

Mitigation

The simplest way to mitigate this issue is to remove the decrement of shutdownVotes from CollectionShutdown::reclaimVote and use only shutdownVoters to track users votes. Function can be modified as follows:

function reclaimVote(address _collection) public whenNotPaused {
    // If the quorum has passed, then we can no longer reclaim as we are pending
    // an execution.
    CollectionShutdownParams storage params = _collectionParams[_collection];
    if (params.canExecute) revert ShutdownQuorumHasPassed();

    // Get the amount of votes that the user has cast for this collection
    uint userVotes = shutdownVoters[_collection][msg.sender];

    // If the user has not cast a vote, then we can revert early
    if (userVotes == 0) revert NoVotesPlacedYet();

    // We delete the votes that the user has attributed to the collection
    delete shutdownVoters[_collection][msg.sender];

    // We can now return their tokens
    params.collectionToken.transfer(msg.sender, userVotes);

    // Notify our stalkers that a vote has been reclaimed
    emit CollectionShutdownVoteReclaim(_collection, msg.sender, userVotes);
}