-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
31a111d
commit bfd3480
Showing
798 changed files
with
67,382 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
Lone Coconut Cat | ||
|
||
Medium | ||
|
||
# Collection Shutdown Can Be Cancelled Through Griefing | ||
|
||
## Summary | ||
|
||
Attempts to sunset a collection can be averted through griefing post-quorum. | ||
|
||
## Vulnerability Detail | ||
|
||
A `CollectionToken` may be sunset by the [`CollectionShutdown`](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol) contract via a permissionless vote via a call to [`start(address)`](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L135C14-L135C40). | ||
|
||
The voting period [can only be started when the collection has a limited circulating supply](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L143C9-L147C116): | ||
|
||
```solidity | ||
// Get the total number of tokens still in circulation, specifying a maximum number | ||
// of tokens that can be present in a "dormant" collection. | ||
params.collectionToken = locker.collectionToken(_collection); | ||
uint totalSupply = params.collectionToken.totalSupply(); | ||
if (totalSupply > MAX_SHUTDOWN_TOKENS * 10 ** params.collectionToken.denomination()) revert TooManyItems(); | ||
``` | ||
|
||
If the vote reaches quorum, it is then possible to shut down the collection. | ||
|
||
This is [gated by the `canExecute` flag](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L234C9-L234C67): | ||
|
||
```solidity | ||
// If we can execute, then we need to fire another event | ||
if (!params.canExecute && params.shutdownVotes >= params.quorumVotes) { | ||
params.canExecute = true; /// @audit collection can be shutdown | ||
emit CollectionShutdownQuorumReached(_collection); | ||
} | ||
``` | ||
|
||
**However, it is possible for a vote to be canceled post-quourm via the** [`cancel(address)`](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L390C14-L390C41) **function**, if the supply of `collectionToken` has increased: | ||
|
||
```solidity | ||
function cancel(address _collection) public whenNotPaused { | ||
// Ensure that the vote count has reached quorum | ||
CollectionShutdownParams memory params = _collectionParams[_collection]; | ||
if (!params.canExecute) revert ShutdownNotReachedQuorum(); /// @audit vote must have passed | ||
// 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()) { /// @audit the permissionless circulating supply of tokens controls gating | ||
revert InsufficientTotalSupplyToCancel(); | ||
} | ||
// Remove our execution flag | ||
delete _collectionParams[_collection]; | ||
emit CollectionShutdownCancelled(_collection); | ||
} | ||
``` | ||
|
||
Notice then, that the number of tokens currently in circulation has the ability to control whether the post-vote quorum can be cancelled. | ||
|
||
An attacker could therefore permissionlessly mint more tokens to satisfy the threshold for cancellation and invalidate the vote prior to finalization by the protocol owner, then immediately liquidate these to reclaim their token. | ||
|
||
**Consequently, any attempt to honestly sunset a collection can be terminated through griefing**. | ||
|
||
## Impact | ||
|
||
Users can be prevented from sunsetting a collection even after a successful vote has taken place, which hinders the capacity of token-aligned stakeholders to administer their collection, for example, to relist the `CollectionToken` for the underlying token with more optimal characteristics (i.e. a lesser/greater denomination). | ||
|
||
It should be noted that not even an admin can circumvent this attack; it must again be put to a public vote, which can be similarly circumvented by an attacker. | ||
|
||
## Code Snippet | ||
|
||
```solidity | ||
/** | ||
* If a shutdown flow has not been triggered and the total supply of the token has risen | ||
* above the threshold, then this function can be called to remove the process and prevent | ||
* execution. | ||
* | ||
* This is done to ensure that collections cannot be marked to shutdown in it's infancy and | ||
* then as more tokens are added then the shutdown is actioned, rugging people that weren't | ||
* aware of the action. | ||
* | ||
* @param _collection The collection address | ||
*/ | ||
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); | ||
} | ||
``` | ||
|
||
https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L379C5-L405C6 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
|
||
When quorum is met for sunsetting a collection, [the protocol `owner` is still required to finalize the process](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/utils/CollectionShutdown.sol#L231C77-L231C86). | ||
|
||
We advise that invalidating a post-quorum attempt to sunset a collection via a call to `cancel(address)` should similarly be restricted to the discretion of the `owner`. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
Modern Metal Butterfly | ||
|
||
High | ||
|
||
# Even after execute has been called, an attacker can reset canExecute to true and cancle to delete the collection which will DOS users claims. | ||
|
||
## Summary | ||
Missing restriction in [```CollectionShutdown::vote```](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L175-L214) function can cause complete DOS of users since it doesn't check that the given collection is already executed. | ||
|
||
## Vulnerability Detail | ||
In ```vote``` function there is a missing check to ensure that the given collection is not yet executed. | ||
|
||
Because of this, an attacker can call ```vote``` even after shutdown has been executed, which will set ```params.canExecute = true;``` and can now call cancle to delete the collection's data. | ||
```javascript | ||
if (!params.canExecute && params.shutdownVotes >= params.quorumVotes) { | ||
params.canExecute = true; | ||
emit CollectionShutdownQuorumReached(_collection); | ||
} | ||
``` | ||
|
||
```javascript | ||
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); | ||
} | ||
``` | ||
|
||
Example: | ||
* Bob is an malicious user, who has just 0.1 of the collection token(0.1 as in e18 decimals, could also be just 1 i.e. dust), | ||
* The other holders vote to shutdown the collection | ||
* quorum is reached and execute is triggered, this ```execute``` function will reset the `params.canExecute` to [`false`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L273). | ||
* Now cancle cannot be called since it checks [`if (!params.canExecute) revert ShutdownNotReachedQuorum();`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L393) | ||
* But, Bob can easily bypass this by calling `vote` with his 0.1 collection tokens, which will reset `params.canExecute` to [`true`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L208C1-L209C38) | ||
* Since `params.canExecute` is reset to `true` Bob can now call `cancle` which will delete the collection params [`delete _collectionParams[_collection];`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L403) | ||
|
||
Now if the users try to call claim, it will revert because they are calling an empty(deleted) collection data; | ||
```javascript | ||
function claim(address _collection, address payable _claimant) public nonReentrant whenNotPaused { | ||
// Ensure our user has tokens to claim | ||
uint claimableVotes = shutdownVoters[_collection][_claimant]; | ||
if (claimableVotes == 0) revert NoTokensAvailableToClaim(); | ||
|
||
// Ensure that we have moved token IDs to the pool | ||
CollectionShutdownParams memory params = _collectionParams[_collection]; | ||
if (params.sweeperPool == address(0)) revert ShutdownNotExecuted(); | ||
|
||
// Ensure that all NFTs have sold from our Sudoswap pool | ||
if (!collectionLiquidationComplete(_collection)) revert NotAllTokensSold(); | ||
|
||
// We can now delete our sweeper pool tokenIds | ||
if (params.sweeperPoolTokenIds.length != 0) { | ||
delete _collectionParams[_collection].sweeperPoolTokenIds; | ||
} | ||
|
||
// Burn the tokens from our supply | ||
params.collectionToken.burn(claimableVotes); | ||
|
||
// Set our available tokens to claim to zero | ||
delete shutdownVoters[_collection][_claimant]; | ||
|
||
// Get the number of votes from the claimant and the total supply and determine from that the percentage | ||
// of the available funds that they are able to claim. | ||
uint amount = params.availableClaim * claimableVotes / (params.quorumVotes * ONE_HUNDRED_PERCENT / SHUTDOWN_QUORUM_PERCENT); | ||
(bool sent,) = _claimant.call{value: amount}(''); | ||
if (!sent) revert FailedToClaim(); | ||
|
||
emit CollectionShutdownClaim(_collection, _claimant, claimableVotes, amount); | ||
} | ||
``` | ||
|
||
|
||
## Impact | ||
Since the collection will be deleted, ```delete _collectionParams[_collection];```, users will not be able to call claim and the eth from selling the nfts will be locked forever in the contract. | ||
|
||
## Code Snippet | ||
```javascript | ||
function vote(address _collection) public nonReentrant whenNotPaused { | ||
// Ensure that we are within the shutdown window | ||
CollectionShutdownParams memory params = _collectionParams[_collection]; | ||
if (params.quorumVotes == 0) revert ShutdownProccessNotStarted(); | ||
|
||
_collectionParams[_collection] = _vote(_collection, params); | ||
} | ||
``` | ||
```javascript | ||
params.canExecute = false; | ||
emit CollectionShutdownExecuted(_collection, pool, _tokenIds); | ||
``` | ||
|
||
## Tool used | ||
Manual Review | ||
|
||
## Recommendation | ||
introduce a new mapping to keep track of executed collections like; | ||
```mapping(address _collection => bool) public isCollectionExecuted;``` | ||
|
||
And then use this to keep track of executed collections | ||
```diff | ||
function execute(address _collection, uint[] calldata _tokenIds) public onlyOwner whenNotPaused { | ||
// Ensure that the vote count has reached quorum | ||
CollectionShutdownParams storage params = _collectionParams[_collection]; | ||
if (!params.canExecute) revert ShutdownNotReachedQuorum(); | ||
|
||
// Ensure we have specified token IDs | ||
uint _tokenIdsLength = _tokenIds.length; | ||
if (_tokenIdsLength == 0) revert NoNFTsSupplied(); | ||
... | ||
... | ||
|
||
+ isCollectionExecuted[_collection] = true; | ||
} | ||
``` | ||
|
||
Now make sure that the given collection is not yet executed in the `vote` function; | ||
```diff | ||
function vote(address _collection) public nonReentrant whenNotPaused { | ||
// Ensure that we are within the shutdown window | ||
CollectionShutdownParams memory params = _collectionParams[_collection]; | ||
if (params.quorumVotes == 0) revert ShutdownProccessNotStarted(); | ||
|
||
+ if (isCollectionExecuted) revert CollectionAlreadyExecutedUse_voteAndClaim() | ||
|
||
_collectionParams[_collection] = _vote(_collection, params); | ||
} | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
Modern Metal Butterfly | ||
|
||
High | ||
|
||
# No function for voters to reclaim their collectionTokens incase the shutdown process was canceled before being executed. | ||
|
||
## Summary | ||
Protocol supports cancelation of shutdown process, but fails to return the `collectionTokens` to the voters who already sent the tokens to the contract as a vote. | ||
|
||
There is a `reclaimVote` function for voters to unvote, but if voters tries to use this to reclaim collectionTokens incase the shutdown process was canceled, it will revert because the `_collectionParams` is already deleted. | ||
|
||
## Vulnerability Detail | ||
Suppose 4 users vote to shutdown a collection, but there was a new deposit and cancel was triggered for the given collection; | ||
```javascript | ||
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); | ||
} | ||
``` | ||
But what happens to those voters who voted 4e18 collectionTokens? | ||
There is not function for them to claim back their collateralTokens. | ||
|
||
The [`reclaimVote`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L356-L377) function is not for this purpose, but still even if they try to use , it will also revert because; | ||
as we can see in the [`cancel`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L390-L405) function it already deletes the `_collectionParams[_collection]`, | ||
and therefore `reclaimVote` will revert in the line [`params.shutdownVotes -= uint96(userVotes);`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L369C9-L369C50) because the [`params.shutdownVotes`](https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/utils/CollectionShutdown.sol#L359) is 0. | ||
|
||
## Impact | ||
Voters will not be able to claim back their collectionTokens and hence their nfts are also locked. | ||
It also doesn't need a malicious intention for this to happen, this can also happen in a very normal situation. | ||
|
||
## Code Snippet | ||
```javascript | ||
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); | ||
} | ||
``` | ||
|
||
## Tool used | ||
Manual Review | ||
|
||
## Recommendation | ||
Introduce a new mapping to keep track of canceled collections and a function for voters to reclaim their collateralTokens; | ||
|
||
```javascript | ||
mapping (address _collection => bool) public wasCollectionCanceled; | ||
|
||
function reclaimVotesIfCanceled(address _collection) external { | ||
if(!wasCollectionCanceled[_collection]) revert collectionNotCanceled(); | ||
|
||
uint userVotes = shutdownVoters[_collection][msg.sender]; | ||
if (userVotes == 0) revert NoVotesPlacedYet(); | ||
|
||
delete shutdownVoters[_collection][msg.sender]; | ||
|
||
params.collectionToken.transfer(msg.sender, userVotes); | ||
} | ||
|
||
``` | ||
|
||
Also set the `wasCollectionCanceled` to true after a collection has been canceled; | ||
```diff | ||
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]; | ||
|
||
+ wasCollectionCanceled[_collection] = true; | ||
emit CollectionShutdownCancelled(_collection); | ||
} | ||
``` |
Oops, something went wrong.