Revolution Protocol - ABAIKUNANBAEV's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L234

Vulnerability details

Impact

In the current implementation of the CultureIndex smart contract, there is a quorumVotes parameter for each pieceId that's needed to be passed for the piece to be dropped. The problem is that this value depends on the totalVotesSupply meaning it depends on the sum of the ERC20 governance tokens and ERC721 verbIds in the system. This can lead to a situation where some users have to obtain more votes than others which creates unfair voting procedure.

Proof of Concept

Let's say there are 2 users who created pieceIds at the totalSupply = 1000, quorumVotesBPS is set to 10% (for simplicity):

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L234

newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

So for them the quorumVotes param is 100.

Another (third) user has deposited more tokens to the system and now the supply is 1100. His quorumVotes param will be set to 110, and his totalVoteWeights got to be greater or equal to the quorumVotes in the params:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L523

 require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped.");

This means that he has to get more total weight than the users who deposited when the total supply was lower. Such situation may lead to several attacks such as when user tries to create pieceId but somebody front-runs him with a huge deposit and his quorumVotes now is bigger than it should be when he called createPiece() at first place. And the user who made such deposit can later withdraw his amount and call createPiece() himself and his quorumVotes will be smaller and, therefore, easier to be passed.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the logic to unify how the quorumVotes for all the users is determined. It can be the system where quorumVotes depends on dynamically changing totalSupply() and not snapshotted at the moment of creation.

Assessed type

Other

Finding Summary

[L-01] verbId parameter is incorrectly checked in getArtPieceById()

Inside of VerbsToken contract there is a function that checks whether the verbId less or equal to _currentVerbId. However, it uses <= operator instead of < as it's done in the CultureIndex contract when checking for pieceId:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L308

require(pieceId < _currentPieceId, "Invalid piece ID");

The problem is that the when pieceId and verbId are created, they are assigned to the id of the _currentPieceId and _currentVerbId accordingly. But the currentVerbId is increased by one when assigning and it cannot be equal to the verbId therefore:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol#L273-276

  function getArtPieceById(uint256 verbId) public view returns (ICultureIndex.ArtPiece memory) {
        require(verbId <= _currentVerbId, "Invalid piece ID");
        return artPieces[verbId];
    }

In the case of the first creation:

verbId = 0
_currentVerbId = 1

Recommendation

Change <= on < when calling getArtPieceById().

[L-02] Reliance on address(this).balance allows other users to send tokens

and always bypass the check in AuctionHouse

Inside of AuctionHouse.sol smart contract, there is a check that compares address(this).balance with reservePrice. And if the reservePrice is less than the balance, then the last bidder will be refunded and verbId will be burnt.

However, this assumption doesn't take into account the fact that the ETH can be sent to the smart contract directly via selfdestruct(), for example. So if the value of ETH sent equals to or becomes bigger than the reservePrice variable, this check will simply always bypass.

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L348-352

if (address(this).balance < reservePrice) { // If contract balance is less than reserve price, refund to the last bidder if (_auction.bidder != address(0)) { _safeTransferETHWithFallback(_auction.bidder, _auction.amount) }

Recommendation

Change the check and remove the address(this).balance as parameter that is used to compare with reservePrice variable.

[L-03] getPastVotes() gives the unfair voting privileges to newly created art pieces

According to the docs, the users can use their voting power to vote on different art pieces only if they had that voting power at the time of creation (block.number).

However, the problem is that this will disincentivize to create art pieces early and everybody would wait until somebody has already created their art pieces and enough number of voters are already present in the system. That could cause a situation where users just wait for each other action and don't create anything:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L313

  uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);

Recommendation

Use block.timestamp to determine whether the user can vote.

[L-04] OTHER mediatype is initialized but it's not validated

Inside of CultureIndex.sol contract, validateMediaType() function is used only to validate TEXT, VIDEO and ANIMATION enum members but it also allows to create media type of type OTHER:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L216

 validateMediaType(metadata);

MediaType enum:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/interfaces/ICultureIndex.sol#L78-85

  enum MediaType {
        NONE,
        IMAGE,
        ANIMATION,
        AUDIO,
        TEXT,
        OTHER
    }

This would allow other users to put OTHER as mediaType and provide image or animationUrl or text with zero length that is validated. Therefore, it can bypass the validateMediatype() check:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L159-168

function validateMediaType(ArtPieceMetadata calldata metadata) internal pure {
        require(uint8(metadata.mediaType) > 0 && uint8(metadata.mediaType) <= 5, "Invalid media type");

        if (metadata.mediaType == MediaType.IMAGE)
            require(bytes(metadata.image).length > 0, "Image URL must be provided");
        else if (metadata.mediaType == MediaType.ANIMATION)
            require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided");
        else if (metadata.mediaType == MediaType.TEXT)
            require(bytes(metadata.text).length > 0, "Text must be provided");
    }

Recommendation

Add the check for OTHER metadata type inside of validateMediaType and check for the length of all three (metadata.image, metadata.animationUrl, metadata.text) in this scenario or just remove this from the types.

[N-01] totalErc20Supply parameter is not used anywhere

When newPiece is created inside of CultureIndex, it's not used anywhere after the initiatialization so it can be easily removed:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L230

 newPiece.totalERC20Supply = erc20VotingToken.totalSupply();

Recommendation

Remove totalErc20Supply param or use it somewhere in the logic.

[N-02] weight should be >= than minWeight

In the current implementation, when checking whether the weight of the voter is bigger than the minimum weight, the following operator is used:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L314

require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");

Recommendation

Change on weight >= minWeight`.

But the name of the minVoteWeight suggests that it's the minimum weight that can be used for voting and, therefore, it should be included in the check as well. Otherwise, it remains confusing.

[N-03] Excessive checks for creators.length

creators.length parameter is checked twice both in CultureIndex and VerbsToken:

CultureIndex.sol:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L213

   uint256 creatorArrayLength = validateCreatorsArray(creatorArray);

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L182

require(creatorArrayLength <= MAX_NUM_CREATORS, "Creator array must not be > MAX_NUM_CREATORS");

So this check would not allow to create art piece with bigger number of creators than MAX_NUM_CREATORS at first place.

VerbsToken.sol:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol#L286-288

  require(
            artPiece.creators.length <= cultureIndex.MAX_NUM_CREATORS(),
            "Creator array must not be > MAX_NUM_CREATORS"
        );

So this check just repeats itself as there is no way for attacker to increase the number of creators somewhere in between creating an art piece and then creating a verbId.

Recommendation

Remove the check from the VerbsToken contract.

[N-04] Hard-coded gas limit can lead to tx revert in some cases

There protocol several times uses hardcoded gas limit that depending on the complexity of the transaction or gas cost changes in Ethereum could result in failed transactions:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L429

success := call(50000, _to, _amount, 0, 0, 0, 0)

Recommendation

Don't use the hard-coded gas limit.

AuctionHouse is not forced to be minter inside of VerbsToken when initializing

Inside of the protocol, only AuctionHouse can call mint() function but the address of the auction contract is not enforced in the initialize() function which may lead to unexpected mistakes:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol#L153

  minter = _minter;

The same thing applies to the CultureIndex and VerbsToken contracts and dropper role.

Recommendation

Enforce the dropper and minter to be the VerbsToken and CultureIndex contracts.

[N-05] Inconsistent checks in AuctionHouse

When calling settleAuction() in AuctionHouse smart contract, there are several if-statements and else-statements inside of which there is some inconsistency:

1. The first if-statement checks whether the address(this).balance < reservePrice. But this is not possible (if the aforementioned attack with forced sent ETH is not realized) as msg.value that the bidder provides in createBid() is always greater than or equal to reservePrice:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L179

require(msg.value >= reservePrice, "Must send at least reservePrice");

2. The first else-statement is realized when address(this).balance >= reservePrice. But if the contract suggests that every ETH will be sent from it and nothing excessive will be sent to it, the scenario in the following check is also not possible:

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L358

     if (_auction.bidder == address(0))

As if address(this).balance >= reservePrice, it means there is a bidder. And if there is a bidder, it cannot be address(0).

Protocol overview

The purpose of the protocol is to make improvements on Nouns DAO making governance more accessible to users as builders will have an opportunity to create their own art piece and sell it instead of buying already generated tokenIds, as in the case with Nouns DAO.

This includes several processes:

1. Creation: users create their own art pieces by providing necessary metadata and the creators array.
2. Voting: community members vote for the art piece they like and the best one is chosen by using max heap data structure.
3. Minting: the most voted art piece is minted as verbId.
4. Distribution: the newly created verbId pieces are distributed via auction procedure where everybody has a chance to buy them and become a community member with a right to vote.

Main contracts flow overview

CultureIndex.sol

MaxHeap contract is used to follow how many voting weight pieceId has and to dynamically change the positions of the corresponding elements if the voting weight changes to, finally, extract the root element.

It's worth noticing that the contract has dropperAdmin which can only be VerbsToken contract that calls dropTopVotedArtPiece().

VerbsToken.sol

In this contract new verbIds are minted by the AuctionHouse (the minter role of the contract).

AuctionHouse.sol

The contract serves as a place where verbIds are being distributed between the users via the auction procedure.

ERC20TokenEmitter.sol

Systemic risks

Governance: some of the governance design decisions can make the system work not as expected. For example, using block.number as the point where the users voting power is snapshoted instead of using current block.timestamp or another mechanism, is prone to creating various difficult situations where users don't mint anything because they wait for the more users to come into the system. This, again, happens because voting power is snapshoted and new vote pieces have an advantage over the older ones as more voters can vote for them. This problem is indicated in one of my findings.

Centralization risks

Centralized parameters: the developers have an ability to set different parameters such as creatorRateBps and entropyRateBps (when initializing contracts of the system) that may affect user experience and the safety of their funds. It's important to make sure that all those variables have reasonable values. Moreover, only the owners may pause the contracts and change the state of the system.

Security approach of the protocol

The protocol follows some of the concepts of Nouns DAO and improves them by making governance more accessible. The developers took into account past Nouns DAO audits to make sure that the same attack vectors don't apply to the system. It's also said that some private audits were conducted. However, it's highly important to audit contracts comprehensively to be confident that nothing is missed. It includes private audits, audit contests and audits from the firms.

Approach taken when evaluating the codebase

1. Overview of the protocol: at first stage, I looked into the protocol's contract very fast making some notes regarding of how the system is functioning overall.

2. Deep dive: after the quick look, I started to review contracts separately trying to form the clear picture in my head of how every action in the system happens and what are the main actors who initiate the system state changes.

3. Running tests: by setting up the testing environment, I made sure that all the tests passed and nothing remained uncovered.

4. Complex attack vectors: finally, after getting the full understanding of the protocol, I began to create some complex attack scenarios in my head trying to find new vulnerabilities.

Conclusion

In conclusion, the proposed protocol aims to enhance the governance model of Nouns DAO by allowing users to create and sell their own art pieces through a multi-step process involving creation, voting, minting, and distribution.

The protocol introduces systemic risks, particularly in its governance design decisions. The use of block.number for snapshotting voting power may lead to unintended consequences, as users may delay minting, anticipating increased participation. This issue could potentially undermine the effectiveness of the governance mechanism.

Centralization risks are also present, as developers have the authority to set critical parameters like creatorRateBps and entropyRateBps, influencing user experience and fund safety. Furthermore, only contract owners can pause the system or alter its state, introducing a level of centralization that may raise concerns among users.

To mitigate potential security threats, the protocol incorporates concepts from Nouns DAO and claims to have undergone past audits, including private ones. However, it is worth mentioning that a comprehensive audit, encompassing private audits, contests, and assessments by reputable firms, is crucial to ensure the system's robustness.

The evaluation process involved an initial overview of the protocol, a deep dive into individual contracts, rigorous testing, and the exploration of complex attack vectors to identify vulnerabilities.

In moving forward, it is recommended to address the highlighted governance and centralization risks, conduct thorough and transparent audits, and consider potential improvements to the protocol's design to enhance its resilience and user trust.

Time spent:

30 hours