Revolution Protocol - hals's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L226-L235 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L357-L359

Vulnerability details

Impact

• Revolution protocol is meant to enable users from adding their art pieces to be voted on and auctioned if the piece passes quorumVotes assigned to it when it was created.

• The voting quorum for each piece varies based on the totalSupply of the ERC20 voting token and the totalSupply of the verbs token at the time when the piece is created:

  newPiece.totalVotesSupply = _calculateVoteWeight(
    erc20VotingToken.totalSupply(),
    erc721VotingToken.totalSupply()
  );
  newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
  //some code...
  newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

• The verbs token (erc721VotingToken) is minted by the AuctionHouse only when an auction is created , and when the auction time ended with a winning bidder; then this bidder will receive this verbs token, and if there's no bidders on the auction; the minted verbs token for that auction is burnt:

  //If no one has bid, burn the Verb
  if (_auction.bidder == address(0)) verbs.burn(_auction.verbId);

• So as can be noticed: at the time the art piece is added; the quorumVotes assigned to that piece will take into account the verbs token that is currently under auction, while this token might be burned if it has no bidders.

• And by knowing that only one auction can exist at a time; the assigned quorumVotes of the art piece can represent a higher threshold than the actual one (if the auction verbs is burned) by a value of (erc721VotingToken.totalSupply() * erc721VotingTokenWeight * 1e18) * (quorumVotesBPS / 10_000).

Proof of Concept

CultureIndex.createPiece function/L226-L235

newPiece.totalVotesSupply = _calculateVoteWeight(
  erc20VotingToken.totalSupply(),
  erc721VotingToken.totalSupply()
);
newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
newPiece.metadata = metadata;
newPiece.sponsor = msg.sender;
newPiece.creationBlock = block.number;
newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

AuctionHouse._settleAuction function/ L357-L359

//If no one has bid, burn the Verb
if (_auction.bidder == address(0)) verbs.burn(_auction.verbId);

Tools Used

Manual Review.

Recommended Mitigation Steps

Check if there's a running auction and deduct 1 from the erc721VotingToken.totalSupply() when assigning the newpiece.quorumVotes.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

• reservePrice represents the minimum price accepted in an auction, so when a user creates a bid on an going auction; the value sent with the function is checked to be greater than the reservePrice.

• The AuctionHouse contract owner (which is the DAO) can change this value at anytime via AuctionHouse.setReservePrice function, and this can be done without checking if there's an ongoing auction or not.

• Changing this value while there's an ongoing auction is unfair for the bidders as it creates unequal opportunities.

• If reservePrice value is increased during an auction: this can block the createBid function when a bidder creates a bid with higher amount as the previous bidder can't be refunded due to this check in the _safeTransferETHWithFallback function (called to refund the previous bidder):

  // Ensure the contract has enough ETH to transfer
  if (address(this).balance < _amount) revert("Insufficient balance");

• If reservePrice value is increased during an auction and no more bids are made after this change: this will result in considering the auction as a failed one because of the following check when settling an auction (the case is present if few bidders have made bids on an auction -low auction amount increment enforced by minBidIncrementPercentage- where the contract balance is almost equal to the increased reservePrice but less than it):

  // Check if contract balance is greater than reserve price
          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);
              }
  
              // And then burn the Noun
              verbs.burn(_auction.verbId);

where the bid amount is returned to the bidder and the verbs token is burnt; so the bidder has lost while he is entitled to win.

Proof of Concept

AuctionHouse.setReservePrice function

    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Enable updating reservePrice only when there's no auction (or bidders on that auction):

    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
+       require(auction.bidder == address(0),"cant update the value during an ongoing auction");
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L193-L195 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/Descriptor.sol#L143-L150

Vulnerability details

Impact

VerbsToken.tokenURI function is intended to return the token uri for an official verbs token, but it was noticed that this function doesn't validate if the token exists before returning the token uri; thus violating ERC-721 standards when not reverting when called with a nonexistent tokenId.

Proof of Concept

VerbsToken.tokenURI function

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

Descriptor.tokenURI function

    function tokenURI(
        uint256 tokenId,
        ICultureIndex.ArtPieceMetadata memory metadata
    ) external view returns (string memory) {
        if (isDataURIEnabled) return dataURI(tokenId, metadata);


        return string(abi.encodePacked(baseURI, tokenId.toString()));
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update VerbsToken.tokenURI function to revert on nonexistent token:

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
+       require(tokenId < _currentVerbId, "token doesnt exist");
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L74-L75 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L429 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L311

Vulnerability details

Impact

• Art pieces are created in the CultureIndex contract with an array of creators of a maximum of 100 addresses.

• Users vote on art pieces, and with each vote; the maxHeap is re-arranged to preserve the characteristics of a max heap where the top voted pieces is set at first the top of the tree as parents and the children below each parent represent the pieces with lower votes, the cost of this operation increases as the number of added pieces is increased.

• Then the top voted art piece can be dropped to be auctioned by the AuctionHouse; where users can create bids, and the winner bid will win the minted verbs tokens and governance ERC20 tokens.

• The process of creating an auction in the AuctionHouse is done only if after settling the previous auction, and when an auction is settled; multiple operations are done to transfer proceeds to the art piece creators and the protocol.

• But it was noticed that the AuctionHouse contract limits the gas amount associated with _createAuction to be > MIN_TOKEN_MINT_GAS_THRESHOLD which equals to 750_000, and with each external call; it limits the gas to 50_000:

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

• But setting such limits on gas can be exploited by griefers and blocking the AuctionHouse, let's take the following scenario:

  1. Multiple art pieces are created with the maximum number of creators; which is 100 address.
  2. One of these art pieces got the highest votes and dropped to be auctioned, where this art piece
  3. The auction got a bidder, and its dead line is reached so any one can call settleCurrentAndCreateNewAuction function to settle and create a new auction by dropping the next top voted art piece.
  4. Settling an auction has multiple operations as transferring ether rewards to the creators array addresses; and with 100 creators and assuming each would consume 50_000 gas, this will total in 500_000 gas for transferring ether only.
  5. And after the auction is settled; _createAuction will check if the remaining gas is > 750_000 to proceed; if not the transaction would revert.
  6. But even if the gas left is > 750_000; it doesn't account for the operations of extracting the top voted art piece from the max heap which might have a large entries that requires more gas to re-arrange.

• Also, if the above scenario is present where it doesn't revert; the cost of transaction would be hefty (knowing that the protocol is going to be deployed on the Ethereum mainnet); which might not be called by anyone even the protocol members as the cost of transaction might be larger than the gained rewards.

Proof of Concept

CultureIndex.MAX_NUM_CREATORS constant

    // Constant for max number of creators
    uint256 public constant MAX_NUM_CREATORS = 100;

AuctionHouse._safeTransferETHWithFallback function/L429

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

AuctionHouse._createAuction function/L311

require(gasleft() >=
  MIN_TOKEN_MINT_GAS_THRESHOLD, "Insufficient gas for creating auction");

Tools Used

Manual Review.

Recommended Mitigation Steps

• Remove MIN_TOKEN_MINT_GAS_THRESHOLD check in the _createAuction contract.
• Limit the number of creators address in CulturalIndex to be way less than 100 (10 for example).
• Limit the size of the MaxHeap that represents the added art piece at a time.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L399-L409 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L164-L170 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/TokenEmitter/TokenEmitterRewards.sol#L12-L18 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L40-L41

Vulnerability details

Impact

• In AuctionHouse contract: the auction is settled if it's end time is passed, and the proceeds of the auction is distributed between the protocol and creators.

• The creators will first receive an ETH amount based on their bps, and this total amount is determined by the entropyRateBps which represents the split of (auction proceeds * creatorRate) that is sent to the creator as ether (or wrapped ether) in basis points.

• Then the remaining creatorsShare - ethPaidToCreators (if any is left) is used to buy governance ERC20 tokens for the creators.

  if (creatorsShare > ethPaidToCreators) {
                      creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
                          vrgdaReceivers,
                          vrgdaSplits,
                          IERC20TokenEmitter.ProtocolRewardAddresses({
                              builder: address(0),
                              purchaseReferral: address(0),
                              deployer: deployer
                          })
                      );
                  }

• When erc20TokenEmitter.buyToken function is called; it will check if the sent value (creatorsShare - ethPaidToCreators) is > minPurchaseAmount and maxPurchaseAmount (which is set as 0.0000001 ether and 50_000 ether); if not, the transaction will revert resulting in blocking AuctionHouse contract.

• This case likely to happen in the following scenarios:

  1. if ethPaidToCreators very large (almost reaching the creatorsShare), and this can happen if the entropyRateBps is set to a high value.
  2. low competition on an auction which result in low bids winning.

• Note: the case where the creatorsShare - ethPaidToCreators > maxPurchaseAmount is ignored as it requires the griefer bidder to bid with a very large amount to block the AuctionHouse contract causing him a huge loss for griefing operation (with an ether price of ~$2260 : this will require creatorsShare - ethPaidToCreators to be greater than 50_000 ether * 2260 ~= $113_000_000).

Proof of Concept

AuctionHouse._settleAuction function/L399-L409

if (creatorsShare > ethPaidToCreators) {
                    creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
                        vrgdaReceivers,
                        vrgdaSplits,
                        IERC20TokenEmitter.ProtocolRewardAddresses({
                            builder: address(0),
                            purchaseReferral: address(0),
                            deployer: deployer
                        })
                    );
                }

ERC20TokenEmitter.buyToken function/L164-L170

        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );

TokenEmitterRewards._handleRewardsAndGetValueToSend function/L12-L18

    function _handleRewardsAndGetValueToSend(
        uint256 msgValue,
        address builderReferral,
        address purchaseReferral,
        address deployer
    ) internal returns (uint256) {
        if (msgValue < computeTotalReward(msgValue)) revert INVALID_ETH_AMOUNT();sss

RewardSplits.computeTotalReward function/L40-L41

function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) {
        if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT();

Tools Used

Manual Review.

Recommended Mitigation Steps

Add another mechanism to handle the described issue without reverting the transaction that will result in blocking the AuctionHouse contract.

Assessed type

DoS

Revolution Protocol QA Report

Summary

Low Severity Findings

[L-01] VerbsToken contract: changing the descriptor will leave the previously minted tokens un-renderd<a id="l-01" ></a>

Details

• In VerbsToken contract: the descriptor is the contract that's responsible of returning tokens uri to be rendered, and this contract can be changed via VerbsToken.setDescriptor function.
• So if this contract is changed; this will leave the previously minted verbs tokens unable to be rendered as the returned uri from calling descriptor.tokenURI will be invalid.

Proof of Concept

VerbsToken.tokenURI function

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

VerbsToken.setDescriptor function

    function setDescriptor(
        IDescriptorMinimal _descriptor
    ) external override onlyOwner nonReentrant whenDescriptorNotLocked {
        descriptor = _descriptor;

        emit DescriptorUpdated(_descriptor);
    }

Recommendation

Either lock the descriptor once initialized or add a mapping to save each minted verbs with the relevant descriptor contract that will render it.

[L-02] VerbsToken contract: changing the minter during an ongoing auction might block the AuctionHouse contract<a id="l-02" ></a>

Details

• AuctionHouse contract supposed to have the minter role in the VerbsToken contract; where it can mint new verbs token when an auction is created, and burn verbs token if the auction fails.

• So if the owner of the VerbsToken changes the minter role during an ongoing auction in the AuctionHouse, this might result in blocking the AuctionHouse if the auction failed as the contract can't burn the minted verbs token for an auction.

Proof of Concept

AuctionHouse._settleAuction function/L348-L361

    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);
            }

            // And then burn the Noun
            verbs.burn(_auction.verbId);
        } else {
            //If no one has bid, burn the Verb
            if (_auction.bidder == address(0))
                verbs.burn(_auction.verbId);
                //If someone has bid, transfer the Verb to the winning bidder
            else verbs.transferFrom(address(this), _auction.bidder, _auction.verbId);

Recommendation

Prevent updating the minter role in VerbsToken if there's an ongoing auction.

[L-03] CulturalIndex.hasVoted function doesn't check if the pieceId exists<a id="l-03" ></a>

Details

CulturalIndex.hasVoted function returns whether an account has voted for an art piece or not regardless of the this piece being existent or not.

Proof of Concept

CultureIndex.hasVoted function

    function hasVoted(uint256 pieceId, address voter) external view returns (bool) {
        return votes[pieceId][voter].voterAddress != address(0);
    }

Recommendation

Update CultureIndex.hasVoted function to check if the id exists or not:

    function hasVoted(uint256 pieceId, address voter) external view returns (bool) {
+       require(pieceId < _currentPieceId, "Invalid piece ID");
        return votes[pieceId][voter].voterAddress != address(0);
    }

[L-04] Users can't vote in the same block the art piec is created<a id="l-04" ></a>

Details

• In CulturalIndex contract: when a user votes on an art piece, the weight of his votes is calculated based on the weight of the past votes of ERC20VotingToken and ERC721VotingToken (which are governance token and verbs NFTs), where _getPastVotes is invoked with the address of the user and the blockNumber of the art piece, and this will invoke the getPastVotes that will check the blocknumber and retrieve the votes weight at the end of that blocknumber:

      function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
          return
              _calculateVoteWeight(
                  erc20VotingToken.getPastVotes(account, blockNumber),
                  erc721VotingToken.getPastVotes(account, blockNumber)
              );
      }

• But if the block number of the art piece is similar to the current block number; the function will revert and the user can't vote:

      function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) {
          VotesStorage storage $ = _getVotesStorage();
          uint48 currentTimepoint = clock();// this will return Time.blockNumber() which is SafeCast.toUint48(block.number)
          if (timepoint >= currentTimepoint) {
              revert ERC5805FutureLookup(timepoint, currentTimepoint);
          }
          return $._delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
      }

Proof of Concept

CultureIndex._getPastVotes function

    function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
        return
            _calculateVoteWeight(
                erc20VotingToken.getPastVotes(account, blockNumber),
                erc721VotingToken.getPastVotes(account, blockNumber)
            );
    }

VotesUpgradeable.getPastVotes function

    function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) {
        VotesStorage storage $ = _getVotesStorage();
        uint48 currentTimepoint = clock();
        if (timepoint >= currentTimepoint) {
            revert ERC5805FutureLookup(timepoint, currentTimepoint);
        }
        return $._delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
    }

Recommendation

Enable voting to start on the same timeblock of the created artpiece:

    function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) {
        VotesStorage storage $ = _getVotesStorage();
        uint48 currentTimepoint = clock();
-       if (timepoint >= currentTimepoint) {
+       if (timepoint > currentTimepoint) {
            revert ERC5805FutureLookup(timepoint, currentTimepoint);
        }
        return $._delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
    }

[L-05] ERC20TokenEmitter.buyToken function: creatorsAddress and treasury can receive tokens without buying it<a id="l-05" ></a>

Details

• In ERC20TokenEmitter.buyToken function: a check is made to ensuer that the caller is not the creatorsAddress or treasury address as they are allowed to have voting ERC20 tokens by minting them part of the bought tokens amount but not via direct buy:

  //prevent treasury from paying itself
  require(msg.sender != treasury &&
    msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

• But these addresses can receive tokens if their addresses are set in the addresses argument of the buyToken function; as there's no check on this addresses array if it has creatorsAddress or treasury addresses when minting them tokens:

      for (uint256 i = 0; i < addresses.length; i++) {
              if (totalTokensForBuyers > 0) {
                  // transfer tokens to address
                  _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
              }
              bpsSum += basisPointSplits[i];
          }

Proof of Concept

ERC20TokenEmitter.buyToken function/ L209-L215

       for (uint256 i = 0; i < addresses.length; i++) {
            if (totalTokensForBuyers > 0) {
                // transfer tokens to address
                _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
            }
            bpsSum += basisPointSplits[i];
        }

Recommendation

Either remove this check that prevents creatorsAddress and treasury from buying tokens, or check the addresses array to skip minting if it contains any of these addresses.

[L-06] ERC20TokenEmitter.buyToken function: totalTokensForCreators is added to the emittedTokenWad without checking if it's really minted<a id="l-06" ></a>

Details

• In ERC20TokenEmitter.buyToken function: the values of the totalTokensForCreators and totalTokensForBuyers represent the amounts of voting tokens to be minted for these addresses and these values are added to the total emittedTokenWad before minting.

• But if the creatorsAddress is address(0); then this address will not be minted the totalTokensForCreators amount:

  if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
    _mint(creatorsAddress, uint256(totalTokensForCreators));
  }

• This will result in an incorrect value of emittedTokenWad as it has counted the un-minted amount of totalTokensForCreators tokens, and this will adversly affect the price of the voting token (the price will increase if the emittedTokenWad is increased to a value ahead of the intended amount to be minted per day).

Proof of Concept

ERC20TokenEmitter.buyToken function/L186-L203

//Transfer ETH to treasury and update emitted
emittedTokenWad += totalTokensForBuyers;
if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators; // @audit: the amount is added before checking if it's minted

//some code...

//Mint tokens for creators
if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
  _mint(creatorsAddress, uint256(totalTokensForCreators));
}

Recommendation

Update buyToken function to account for the totalTokensForCreators amount after minting it to the creatorsAddress:

//Transfer ETH to treasury and update emitted
emittedTokenWad += totalTokensForBuyers;
-if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

//some code...

//Mint tokens for creators
if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
  _mint(creatorsAddress, uint256(totalTokensForCreators));
+ emittedTokenWad += totalTokensForCreators;
}

[L-07] The protocol might be eventually not gaining auction profits as the minCreatorRateBps can be increased only<a id="l-07" ></a>

Details

• The creatorRateBps represents the split of the winning bid that is reserved for the creator of the Verb in basis points, and this value can be changed by the DAO via AuctionHouse.setCreatorRateBps function; where the new value is checked if it's > minCreatorRateBps (which represents the all time minimum split of the winning bid that is reserved for the creator of the Verb in basis points) before being assigned.

  require(_creatorRateBps >=
    minCreatorRateBps, "Creator rate must be greater than or equal to minCreatorRateBps");

• And the value of the minCreatorRateBps can be updated by the DAO via calling AuctionHouse.setMinCreatorRateBps function; but it was noticed that the minimum value can be updated if it's greater than the previous one:

  require(_minCreatorRateBps >
    minCreatorRateBps, "Min creator rate must be greater than previous minCreatorRateBps");

• This will result in minCreatorRateBps never being decreased, and the creatorRateBps will follow this trend as well, so this will result in the protocol gaining less and less of bidding proceeds with each update of the creatorRateBps (as it will be incremented only if the minCreatorRateBps is updated ==> and the higher the creatorRateBps the lower the gains of the protocol from auctions).

Proof of Concept

AuctionHouse.setMinCreatorRateBps function

    function setMinCreatorRateBps(uint256 _minCreatorRateBps) external onlyOwner {
        require(_minCreatorRateBps <= creatorRateBps, "Min creator rate must be less than or equal to creator rate");
        require(_minCreatorRateBps <= 10_000, "Min creator rate must be less than or equal to 10_000");

        //ensure new min rate cannot be lower than previous min rate
        require(
            _minCreatorRateBps > minCreatorRateBps,
            "Min creator rate must be greater than previous minCreatorRateBps"
        );

        minCreatorRateBps = _minCreatorRateBps;

        emit MinCreatorRateBpsUpdated(_minCreatorRateBps);
    }

Recommendation

Update setMinCreatorRateBps function to accept values less than the previously assigned value:

    function setMinCreatorRateBps(uint256 _minCreatorRateBps) external onlyOwner {
        require(_minCreatorRateBps <= creatorRateBps, "Min creator rate must be less than or equal to creator rate");
        require(_minCreatorRateBps <= 10_000, "Min creator rate must be less than or equal to 10_000");

-       //ensure new min rate cannot be lower than previous min rate
-       require(
-           _minCreatorRateBps > minCreatorRateBps,
-           "Min creator rate must be greater than previous minCreatorRateBps"
-       );

        minCreatorRateBps = _minCreatorRateBps;

        emit MinCreatorRateBpsUpdated(_minCreatorRateBps);
    }

[L-08] ERC20TokenEmitter contrat: block.timestamp can be manipulated by miners <a id="l-08" ></a>

Details

• ERC20TokenEmitter contract functions use block.timestamp to evaluate the price of the ERC20 voting tokens, where the price depends on the number of sold tokens per day, and if the number is ahead a limit set by the VRGDAC contract then the price increases and if it's behind the scheduled limit then the price decreases.

• block.timestamp is used to calculate the passed days (timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime)), and this can be manupulated by miners to manipulate the price of the voting token:

  • if the sold token amount is ahead of the schedule: then the token price will increase, but if a malicious miner decreases block.timestamp , then the price would return normal.
  • if the sold token amount is ahead of the schedule: then the token price will increase, and if a malicious miner increases block.timestamp , then the price would be decreased
  • if the sold token amount is behind the schedule: then the token price will decrease, but if a malicious miner decreases block.timestamp , then the price would return normal.
  • if the sold token amount is behind the schedule: then the token price will decrease, and if a malicious miner increases block.timestamp , then the price would be decreased more.

Proof of Concept

ERC20TokenEmitter.buyTokenQuote function/L243

timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),

ERC20TokenEmitter.getTokenQuoteForEther function/L260

timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),

ERC20TokenEmitter.getTokenQuoteForPayment function/L277

timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),

Recommendation

block.number can be used instead of block.timestamp as it can't be manipulated by miners.

[L-09] AuctionHouse contrat: users can create bids while the contract is paused <a id="l-09" ></a>

Details

Users can create bids on an auction while the contract is paused.

Proof of Concept

ERC20TokenEmitter.createBid function

 function createBid(uint256 verbId, address bidder) external payable override nonReentrant {

Recommendation

Add whenNotPaused modifier to the createBid function:

- function createBid(uint256 verbId, address bidder) external payable override nonReentrant {
+ function createBid(uint256 verbId, address bidder) external payable override nonReentrant whenNotPaused  {

Informational Findings

[I-01] CulturalIndex.createPiece: creatorArray could have duplicate addresses<a id="i-01" ></a>

Details

CulturalIndex.createPiece function enables anyone from adding art pieces to be voted on and auctioned, and the proceeds of the auction is distributed between the deployer (the one who added the art piece) and the creators which their addresses are appeneded to the art piece as the pieces[pieceId].creators, but it was noticed that the creatorArray is only checked if any of the addresses is a zero address, but there's no check for duplicate addresses.

Proof of Concept

CultureIndex.validateCreatorsArray function

  function validateCreatorsArray(CreatorBps[] calldata creatorArray) internal pure returns (uint256) {
        uint256 creatorArrayLength = creatorArray.length;
        //Require that creatorArray is not more than MAX_NUM_CREATORS to prevent gas limit issues
        require(creatorArrayLength <= MAX_NUM_CREATORS, "Creator array must not be > MAX_NUM_CREATORS");

        uint256 totalBps;
        for (uint i; i < creatorArrayLength; i++) {
            require(creatorArray[i].creator != address(0), "Invalid creator address");
            totalBps += creatorArray[i].bps;
        }

        require(totalBps == 10_000, "Total BPS must sum up to 10,000");

        return creatorArrayLength;
    }

Recommendation

Ensure that the creatorArray has no duplicate addresses.

[I-02] Wrong check placement in ERC20TokenEmitter.buyToken function<a id="i-02" ></a>

Details

A chek made in ERC20TokenEmitter.buyToken function to ensure that the totalTokensForBuyers > 0 is placed inside the for-loop while it should only present once before entering the loop.

Proof of Concept

ERC20TokenEmitter.buyToken function/ L209-L215

       for (uint256 i = 0; i < addresses.length; i++) {
            if (totalTokensForBuyers > 0) {
                // transfer tokens to address
                _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
            }
            bpsSum += basisPointSplits[i];
        }

Recommendation

+   if (totalTokensForBuyers > 0) {
       for (uint256 i = 0; i < addresses.length; i++) {
-           if (totalTokensForBuyers > 0) {
                // transfer tokens to address
                _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
-           }
            bpsSum += basisPointSplits[i];
        }
+    }

[I-03] creatorDirectPayment is sent to the creatorsAddress without checking if the address is not address(0)<a id="i-03" ></a>

Details

In ERC20TokenEmitter.buyToken function; the creatorDirectPayment is sent directly to the creatorsAddress without checking if that address != address(0) which will result in losing these sent payments.

Proof of Concept

ERC20TokenEmitter.buyToken function/L194-L198

        //Transfer ETH to creators
        if (creatorDirectPayment > 0) {
            (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
            require(success, "Transfer failed.");
        }

Recommendation

        //Transfer ETH to creators
-       if (creatorDirectPayment > 0) {
+       if (creatorDirectPayment > 0 && creatorsAddress != address(0)) {
            (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
            require(success, "Transfer failed.");
        }

[I-04] Incorrect documentation for MaxHeap.insert function<a id="i-04" ></a>

Details

The documentation of the function indicates that the heap will revert if it's full, while there's no maximum value set for the size of the heap to compare against to check if it's full or not:

```javascript
/// @dev The function will revert if the heap is full
```

Proof of Concept

MaxHeap.insert function

   /// @notice Insert an element into the heap
    /// @dev The function will revert if the heap is full
    /// @param itemId The item ID to insert
    /// @param value The value to insert
    function insert(uint256 itemId, uint256 value) public onlyAdmin {
        heap[size] = itemId;
        valueMapping[itemId] = value; // Update the value mapping
        positionMapping[itemId] = size; // Update the position mapping

        uint256 current = size;
        while (current != 0 && valueMapping[heap[current]] > valueMapping[heap[parent(current)]]) {
            swap(current, parent(current));
            current = parent(current);
        }
        size++;
    }

Recommendation

Update the documentation to match function intention.

[I-05] AuctionHouse._settleAuction function contains excessive calling for a storage variable<a id="i-05" ></a>

Details

In AuctionHouse._settleAuction function: the creatorsShare * entropyRateBps is called with each creator payment calculation in a for-loop while it should be calculated only once and used:

```javascript
uint256 paymentAmount = (creatorsShare _ entropyRateBps _ creator.bps) / (10_000 \* 10_000);
```

Proof of Concept

AuctionHouse._settleAuction function / L390

uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);

Recommendation

        if (creatorsShare > 0 && entropyRateBps > 0) {
+           uint256 totalEth=creatorsShare * entropyRateBps;
                    for (uint256 i = 0; i < numCreators; i++) {
                        ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
                        vrgdaReceivers[i] = creator.creator;
                        vrgdaSplits[i] = creator.bps;

                        //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
-                       uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);

+                       uint256 paymentAmount = (totalEth * creator.bps) / (10_000 * 10_000);
                        ethPaidToCreators += paymentAmount;

                        //Transfer creator's share to the creator
                        _safeTransferETHWithFallback(creator.creator, paymentAmount);
                    }
                }

[I-06] Incorrect documentation for CulturIndex.getVotes function<a id="i-06" ></a>

Details

The documentation of function indicates that it will return the voting power of a voter at the current block, while it actually returns the value in the most recent checkpoint which doesn't necessarily be the current block

```javascript
@notice Returns the voting power of a voter at the current block.
```

Proof of Concept

CultureIndex.getVotes function

  /**
     * @notice Returns the voting power of a voter at the current block.
     * @param account The address of the voter.
     * @return The voting power of the voter.
     */
    function getVotes(address account) external view override returns (uint256) {
        return _getVotes(account);
    }

Recommendation

Update the documentation to match function intention.

[I-07] Incorrect documentation for CultureIndex.getPastVotes function<a id="i-07" ></a>

Details

The documentation of the function indicates that it will return the voting power of a voter at the current block, while it actually returns the voting power at a specific blockNumber:

```javascript
 * @notice Returns the voting power of a voter at the current block.
```

Proof of Concept

CultureIndex.getPastVotes function

    /**
     * @notice Returns the voting power of a voter at the current block.
     * @param account The address of the voter.
     * @return The voting power of the voter.
     */
    function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) {
        return _getPastVotes(account, blockNumber);
    }

Recommendation

Update the documentation to match function intention.

[I-08] Redundant check in CultureIndex.topVotedPieceId function<a id="i-08" ></a>

Details

The function checks if maxHeap.size() > 0 before calling maxHeap.getMax(), while the same check is done in the maxHeap.getMax() function.

Proof of Concept

CultureIndex.topVotedPieceId function

    function topVotedPieceId() public view returns (uint256) {
        require(maxHeap.size() > 0, "Culture index is empty");
        //slither-disable-next-line unused-return
        (uint256 pieceId, ) = maxHeap.getMax();
        return pieceId;
    }

MaxHeap.getMax function

    function getMax() public view returns (uint256, uint256) {
        require(size > 0, "Heap is empty");
        return (heap[0], valueMapping[heap[0]]);
    }

Recommendation

Remove the redundant check in the topVotedPieceId function:

    function topVotedPieceId() public view returns (uint256) {
-       require(maxHeap.size() > 0, "Culture index is empty");
        //slither-disable-next-line unused-return
        (uint256 pieceId, ) = maxHeap.getMax();
        return pieceId;
    }

[I-09] Redundant check in CultureIndex._verifyVoteSignature function<a id="i-09" ></a>

Details

Thhere's a check in this function to ensure that from address is not zero address:

```javascript
if (from == address(0)) revert ADDRESS_ZERO();
```

while this check is implicitly done again in the next line:

```javascript
if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE();
```

Proof of Concept

CultureIndex._verifyVoteSignature function/L438

if (from == address(0)) revert ADDRESS_ZERO();

Recommendation

Remove this redundant check:

-if (from == address(0)) revert ADDRESS_ZERO();

[I-10] Incorrect comment line in _verifyVoteSignature function<a id="i-10" ></a>

Details

A comment indicating that the check is made on to address instead of from:

```javascript
// Ensure to address is not 0
    if (from == address(0)) revert ADDRESS_ZERO();
```

Proof of Concept

CultureIndex._verifyVoteSignature function/L437

// Ensure to address is not 0

Recommendation

-   // Ensure to address is not 0
+   // Ensure from address is not 0
        if (from == address(0)) revert ADDRESS_ZERO();

[I-11] A check needs to be refactored in CultureIndex._vote<a id="i-11" ></a>

Details

This line of code:

```javascript
require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");
```

can be refactored as follows to decrease the number of operations by one:

```javascript
require(votes[pieceId][voter].voterAddress == address(0), "Already voted");
```

Proof of Concept

CultureIndex._vote function/L311

require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");

Recommendation

-require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");
+require(votes[pieceId][voter].voterAddress == address(0), "Already voted");

[I-12] VerbsToken._authorizeUpgrade function has a commited documentation<a id="i-12" ></a>

Details

It seems like the function documentation is re-commited by mistake (as it matches the same documentation for that function all over the protocol contracts).

Proof of Concept

CultureIndex._authorizeUpgrade function documentation

    // /// @notice Ensures the caller is authorized to upgrade the contract and that the new implementation is valid
    // /// @dev This function is called in `upgradeTo` & `upgradeToAndCall`
    // /// @param _newImpl The new implementation address
    function _authorizeUpgrade(address _newImpl) internal view override onlyOwner {

Recommendation

Consider either removing the committed function documentation or removing the second commit.

[I-13] ERC20TokenEmitter.setCreatorsAddress function has a redundant modifier<a id="i-13" ></a>

Details

ERC20TokenEmitter.setCreatorsAddress function has nonReentrant modifier; while using this modifier is redundant in this function as it's a setter and doesn't make any extrenal calls.

Proof of Concept

ERC20TokenEmitter.setCreatorsAddress function

function setCreatorsAddress(address _creatorsAddress) external override onlyOwner nonReentrant {

Recommendation

Consider removing nonReentrant modifier from ERC20TokenEmitter.setCreatorsAddress function.

Recommendation Findings

[R-01] Add a function to enable users from disabling their signatures<a id="r-01" ></a>

Details

• CulturalIndex contract has a functionality that enables users to vote onbehalf of other users on specific art pieces by signatures; but if it was noticed that there's no functionality to revoke/dinvalidate signatures.

• Signatures got invalidated after using them (as the nonces of the user is increased and the votehash will result in a wrong recovered address) or after the deadline of that signature is passed.

• So whenever a user changes his mind on voting on a specific artpiece after creating a signature; he can't do so as there's no mechanism to invalidate signatures.

Recommendation

Add a function in the CulturalIndex contract to invalidate signatures by increasing the nonce of the user:

function invalidateSignature() public{
    nonces[msg.sender]++;
}

[R-02] CultureIndex contract: signatures become invalid if one of the signed art pieces is dropped<a id="r-02" ></a>

Details

• CultureIndex contract has a functionality to enable anyone from executing a vote on a specific art pieces by signature, and each signature has a deadline after which the signature becomes invalid.

• And in the _vote function; the art piece is checked if dropped (auctioned) before voting and if so; the transaction reverts:

  require(!pieces[pieceId].isDropped, "Piece has already been dropped");

• So if the signature has any artpiece that has been dropped after the signature created; it becomes invalid before its deadline.

Proof of Concept

CultureIndex._vote function/L310

require(!pieces[pieceId].isDropped, "Piece has already been dropped");

Recommendation

Update _vote function to return false instead of reverting on a dropped art piece.

[R-03] Pause voting in CulturalIndex contract if the AuctionHouse contract is paused <a id="r-03" ></a>

Details

Pausing the AuctionHousecontract means that the top voted art piece can't be extracted to be auctioned; while in the same time voting in the CulturalIndex contract is not affected by this pause; this will result in unequal/unfair opportunities for art piec deployers as some of them might be the top voted and elligible for auction at the time the aution is paused, then when the auction is unpaused they might lose their top voted positions and other art pieces are auctioned before them.

Recommendation

Pause voting if the auction house is paused.