Revolution Protocol - 0xCiphky's results

Lines of code

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

Vulnerability details

Summary:

The buyToken function in the ERC20TokenEmitter contract is designed to send out all ETH received in a transaction (confirmed by devs). However, a portion of the ETH is inadvertently left in the contract, which subsequently becomes locked as there is no mechanism to withdraw it.

Vulnerability Details:

To understand this issue, let's examine the buyToken function, focusing on how ETH and tokens are distributed from a transaction.

Step 1: Deduct protocol rewards

The function first allocates protocol rewards, which are 2.5% of the total transaction value.

• _handleRewardsAndGetValueToSend = 2.5% of Total (msg.value)

function buyToken(
        ...
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        ...
        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );
        ...
    }

Step 2: Calculate toPayTreasury pay

Next, the amount of ETH to pay the treasury is calculated based on creatorRateBps.

• msgValueRemaining = Total (msg.value) - _depositPurchaseRewards
• toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

function buyToken(
        ...
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        ...
        //Share of purchase amount to send to treasury
        uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;
        ...
    }

Step 3: Calculate creator pay

The creator's share is then divided into ETH (creatorDirectPayment) and tokens. The ETH part is sent directly to the creator, while the token part is minted accordingly.

• creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
  • Amount of ETH creator address gets
• if ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
  • getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
    • Amount of Tokens creator address gets

function buyToken(
        ...
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        ...
        //Share of purchase amount to reserve for creators
        //Ether directly sent to creators
        uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
        //Tokens to emit to creators
        int256 totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int256(0);
	...
    }

Step 4: Calculate buyers tokens in ETH

The final step involves calculating the buyer's tokens, which is equivalent to the treasury’s share in ETH.

• totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int256(0);
  • Amount of Tokens buyer address gets

function buyToken(
        ...
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        ...
        // Tokens to emit to buyers
        int256 totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int256(0);
	...
    }

In summary, the total ETH received is divided among protocol rewards, the creator's share, and the treasury. Focusing specifically on the creator's share, it is further split into two components: the creatorDirectPayment and the totalTokensForCreators. While the creatorDirectPayment is directly sent to the creator in ETH, the totalTokensForCreators are allocated in the form of minted tokens. However, the issue arises with the ETH value corresponding to these totalTokensForCreators. This portion of ETH, despite being accounted for in tokens, remains unallocated and unused within the contract.

Proof Of Concept

function testBuyTokenRemainingETH() public {
        vm.startPrank(address(0));

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);

        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        vm.deal(address(0), 100000 ether);

        vm.stopPrank();
        // set setCreatorsAddress
        vm.prank(address(dao));
        erc20TokenEmitter.setCreatorsAddress(address(100));

        // change creatorRateBps to 1000
        vm.prank(address(dao));
        erc20TokenEmitter.setCreatorRateBps(1000);

        // set entropyRateBps to 1000
        vm.prank(address(dao));
        erc20TokenEmitter.setEntropyRateBps(1000);

        assertEq(address(erc20TokenEmitter).balance, 0, "Contract starts with 0 balance");

        vm.startPrank(address(0));

        erc20TokenEmitter.buyToken{value: 1e18}(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        // creator bal + treasury bal + remaining eth in contract + protocol fees = 1e18
        assertEq(
            address(erc20TokenEmitter.creatorsAddress()).balance + address(erc20TokenEmitter.treasury()).balance
                + address(erc20TokenEmitter).balance + 2.5e16,
            1e18,
            "Creator balance + treasury balance + remaining eth should be 1e18"
        );

        // remaining eth in contract greater than 0
        assertGt(address(erc20TokenEmitter).balance, 0, "Remaining ETH should be 0");
    }

Impact:

A portion of the ETH, instead of being distributed, remains within the contract with no defined pathway for its release. This results in the ETH being effectively stuck in the contract.

• Severity: High. This issue leads to a loss of funds for the protocol, as the undistributed ETH accumulates over time with each transaction.
• Likelihood: High. The buyToken function is used often, which means this issue will likely happen repeatedly.

Tools Used:

• Manual analysis
• Foundry

Recommendation:

Revise the function's logic to guarantee that all ETH received by the contract is fully distributed in each transaction, specifically addressing the ETH equivalent of the totalTokensForCreators.

Assessed type

Other

Judge has assessed an item in Issue #341 as 2 risk. The relevant finding follows:

L03: Auction parameters can be changed during an auction

Lines of code

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

Vulnerability details

Summary:

The CultureIndex contract permits the creation of new art pieces through the createPiece function. These pieces are subject to voting and potential minting based on the highest votes received. Voting on an art piece is only permitted if a user owned or was delegated the respective ERC20 tokens of ERC721 token during or before the creation of the respective art piece.

function createPiece(ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray)
        public
        returns (uint256)
    {
        ...
        ArtPiece storage newPiece = pieces[pieceId];

        newPiece.pieceId = pieceId;
        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;
				...
    }

While this voting system safeguards against certain attacks, it inadvertently creates a disadvantage for pieces created earlier. Art pieces minted at different times can accumulate differing maximum vote counts, giving a distinct advantage to pieces created later, as the total token supply increases over time.

Consider the following scenario:

• User A creates an art piece early when the total supply is 10 tokens.
• User B creates an art piece later when the total supply has increased to 100 tokens.
• Once User B's piece receives 11 token votes, User A's piece can never be the highest voted, as the maximum possible votes for it are capped at 10 due to the token supply at its creation time.

Impact:

• Early creators are incentivized to recreate their art pieces, leading to an unnecessary increase in the total number of art pieces and additional load on the system.
• Creators of early pieces face a significant disadvantage compared to later creators.

Tools Used:

• Manual analysis

Recommendation:

Implement a feature that permits creators of early art pieces, particularly those that have not yet been voted to the top, to delete and recreate their submissions. This would enable these pieces to be reintroduced with a vote capacity aligned with the current total token supply, thereby levelling the playing field.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary:

The CultureIndex contract facilitates a snapshot approach voting mechanism for art pieces. This method allows users to vote on art pieces using the ERC20 and ERC721 tokens they held or were delegated at or before the specific snapshot time.

Furthermore, the protocol implements a quorum requirement for art pieces to progress to the auction stage. This requirement ensures that a piece must reach a community consensus before it can be minted. The quorum threshold is influenced by the total vote supply available at the moment the art piece is created.

While effective in scenarios with sufficient supply, this system can encounter problems when the token supply is minimal or nonexistent.

function createPiece(ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray)
        public
        returns (uint256)
    {
	...
        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;
	...
    }

totalVotesSupply is 0

In cases where the total vote supply is zero, the quorum becomes ineffective, leading to potential bypassing of quorum restrictions. Additionally, these pieces will be ineligible for voting due to the time-based snapshot voting mechanism.

totalVotesSupply is low

When the total vote supply is low, it can lead to an art piece being indefinitely locked from reaching the auction stage due to the quorum threshold. Specific scenarios contributing to this problem include:

• An NFT initially counted in the total supply could later be burned, but since the total supply recorded at the creation time remains unchanged, it continues to influence the quorum.
• Early buyers making significant purchases could inadvertently control the fate of an art piece. If their token holdings exceed the quorum threshold, they can single-handedly prevent an art piece from being auctioned.
• The DAO might mint tokens for governance purposes, which may not be used for voting on art pieces. Additionally a portion of every buyToken transaction contributes to the DAO through the creator address. These tokens, while contributing to the total supply, do not actively participate in the voting process.

Impact:

This issue could make it difficult for early creators to meet the quorum threshold or compete with later creators, as their maximum achievable votes are limited and can voting be more easily influenced when the total vote supply is low.

Tools Used:

• Manual analysis

Recommendation:

Ensure that the ERC20TokenEmitter contract is deployed and operational prior to enabling the creation of art pieces. This allows the token to reach a certain supply threshold and a diverse holder base, mitigating the issues associated with low supply.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary:

The _settleAuction function in the AuctionHouse contract is responsible for finalizing auctions, transferring art pieces to the highest bidder, and paying out the creator and owner. Generally, this function operates as intended, but it fails to handle an edge case where entropyRateBps is set to zero.

Vulnerability Details:

The _settleAuction function calls the buyToken function from the ERC20TokenEmitter contract to purchase tokens and pay the creators of the art piece. The vrgdaSplits and vrgdaReceivers arrays, which are essential for this process, are only set if the condition “creatorsShare > 0 && entropyRateBps > 0” is met. If entropyRateBps is zero, indicating that creators are to be paid entirely in tokens, these arrays remain uninitialized. This results in them being passed with default (zero) values to the buyToken function.

function _settleAuction() internal {
        ...
                //Build arrays for erc20TokenEmitter.buyToken
                uint256[] memory vrgdaSplits = new uint256[](numCreators);
                address[] memory vrgdaReceivers = new address[](numCreators);
                ...
                uint256 ethPaidToCreators = 0;
                //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken
                if (creatorsShare > 0 && entropyRateBps > 0) {
                    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;
			...
                    }
                }
                //Buy token from ERC20TokenEmitter for all the creators
                if (creatorsShare > ethPaidToCreators) {
                    creatorTokensEmitted = erc20TokenEmitter.buyToken{value: creatorsShare - ethPaidToCreators}(
                        vrgdaReceivers,
                        vrgdaSplits,
                        IERC20TokenEmitter.ProtocolRewardAddresses({
                            builder: address(0),
                            purchaseReferral: address(0),
                            deployer: deployer
                ...
    }

As a consequence, the erc20TokenEmitter.buyToken function will always revert when attempting to mint tokens for creators, because its internal mint function reverts when the receiver is the zero address.

function _mint(address account, uint256 value) internal override {
        if (account == address(0)) {
            revert ERC20InvalidReceiver(address(0));
        }
        _update(address(0), account, value);
    }

Given that the setEntropyRateBps function allows for a zero value, it's a valid edge case that should be accounted for in the _settleAuction function.

function setEntropyRateBps(uint256 _entropyRateBps) external onlyOwner {
        require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");

        entropyRateBps = _entropyRateBps;
        emit EntropyRateBpsUpdated(_entropyRateBps);
    }

Proof Of Concept

function testSettleAuctionZeroEntropyRate() public {
        // set entropy rate to 0
        auction.setEntropyRateBps(0);

        createDefaultArtPiece();
        auction.unpause();

        address recipient = address(0x123); // Some EOA address
        uint256 amount = 1 ether;

        auction.transferOwnership(recipient);

        vm.startPrank(recipient);
        auction.acceptOwnership();

        vm.startPrank(address(auction));
        vm.deal(address(auction), amount);
        auction.createBid{value: amount}(0, address(this)); // Assuming first auction's verbId is 0
        //go in future
        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction
        // reverts as creator array not set, zero address
        vm.expectRevert(abi.encodeWithSelector(ERC20InvalidReceiver.selector, address(0)));
        auction.settleCurrentAndCreateNewAuction();
    }

Impact:

If the entropyRateBps is set to zero, the _settleAuction function will invariably fail until the contract owner changes the entropyRateBps to a non-zero value.

• Severity: Medium. The system will revert when setting entropyRateBps to zero, even though it falls within the valid range.
• Likelihood: Low. The protocol is unlikely to often use this specific setting.

Tools Used:

• Manual analysis
• Foundry

Recommendation:

To resolve this issue, the initialization of the vrgdaSplits and vrgdaReceivers arrays should occur outside the conditional statement that checks for entropyRateBps > 0. This ensures that even when entropyRateBps is zero, the correct addresses and splits are provided to the buyToken function, thereby allowing the function to execute successfully regardless of the entropyRateBps value.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary:

The CultureIndex contract allows off chain voting by users assigning their votes via signature to others. The signature is verified using a typehash that includes the user's address, specific pieceIds, nonce, and deadline. The trusted party/user can then vote on behalf of users.

Per the EIP-712 standard, arrays should be encoded by concatenating their elements and hashing the result with keccak256:

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

However, the current implementation in the contract does not encode the array in this manner. Instead, the pieceIds array is passed directly:

function _verifyVoteSignature(
        ...
    ) internal returns (bool success) {
        ...
        voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline));
        ...
    }

As a result, the signature verification process is not compliant with EIP-712. Users, contracts, or dApps encoding arrays as per EIP-712 specifications will produce different signatures, leading to a failure of the above functions when executed.

Impact:

• Severity: Low. The discrepancy in encoding leads to a high likelihood of signature mismatches, disrupting functionality for users.
• Likelihood: High. EIP-712 is a widely recognized standard

Tools Used:

• Manual analysis

Recommendation:

To rectify this issue and align with EIP-712 standards, the signature verification process in the contract should be updated to encode arrays correctly as outlined in EIP-712.

Assessed type

Other

L01: Metadata not properly validated

Severity:

• Low Risk

Relevant GitHub Links:

• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L209
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L159
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/interfaces/ICultureIndex.sol#L78

Summary:

The CultureIndex contract permits the creation of new art pieces through the createPiece function. These pieces are subject to voting and potential minting based on the highest votes received. The createPiece function sets forth certain requirements for the art piece's metadata:

Requirements:

• metadata must include name, description, and image. Animation URL is optional.
• creatorArray must not contain any zero addresses.
• The sum of basis points in creatorArray must be exactly 10,000. */

However, the createPiece function currently lacks verification for the presence of a name and description in the metadata fields.

    function createPiece(ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray)
        public
        returns (uint256)
    {
        uint256 creatorArrayLength = validateCreatorsArray(creatorArray);

        // Validate the media type and associated data
        validateMediaType(metadata);
        ...
    }

Additionally, the validateMediaType function is called within createPiece to ensure requirements are met depending on the MediaType chosen. However, it does not cover all cases. A user can select from the following types:

// Enum representing different media types for art pieces.
    enum MediaType {
        NONE,
        IMAGE,
        ANIMATION,
        AUDIO,
        TEXT,
        OTHER
    }

While the IMAGE, ANIMATION, and TEXT fields are checked to ensure a valid value is provided, the AUDIO type, which should fall under ANIMATION, is not checked.

• OpenSea Metadata Standards: https://docs.opensea.io/docs/metadata-standards

/**
     * Requirements:
     * - The media type must be one of the defined types in the MediaType enum.
     * - The corresponding media data must not be empty.
     */
    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");
        }
    }

As a result, it is currently possible for art pieces to be created with missing fields in the metadata. Furthermore, as these pieces are auctioned off, an art piece that doesn’t follow the protocol's requirements could inadvertently be auctioned off for a substantial amount.

Impact:

• Severity: Medium. It is possible for art pieces with incomplete metadata to be created and auctioned.
• Likelihood: Low. It is unlikely that an art piece with missing metadata would receive enough votes to be minted.

Tools Used:

• Manual analysis

Recommendation:

Add the following requirements to ensure the metadata requirements are followed by users when creating art pieces

    function createPiece(ArtPieceMetadata calldata metadata, CreatorBps[] calldata creatorArray)
        public
        returns (uint256)
    {
        require(bytes(metadata.name).length > 0, "name must be provided"); //add here
        require(bytes(metadata.description).length > 0, "description must be provided"); //add here
        ...
    }

    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 || metadata.mediaType == MediaType.AUDIO) { //add here
            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");
        }
    }

L02: Immutable variables not preserved between upgrades

Severity:

• Low Risk

Relevant GitHub Links:

• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L85
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L85
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/Descriptor.sol#L60
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/MaxHeap.sol#L23
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L109

Summary:

The protocol employs the following upgradeable contracts: CultureIndex, MaxHeap, Descriptor, AuctionHouse, and VerbsToken. Despite these contracts being upgradeable they still initialize immutable variables through constructors.

This approach is not recommended for proxy contracts because immutable variables do not persist through upgrades. If the implementation contract is upgraded without inheriting the first version or integrating the immutable variables into the new implementation, they will not be available in the new version, unlike storage variables that reside in the proxy contract.

/// @notice The contract upgrade manager
    IRevolutionBuilder public immutable manager;

    ///                                                          ///
    ///                         CONSTRUCTOR                      ///
    ///                                                          ///

    /// @param _manager The contract upgrade manager address
    constructor(address _manager) payable initializer {
        manager = IRevolutionBuilder(_manager);
    }

Impact:

This design can lead to unexpected behaviour in the upgraded contract versions, if the immutable values set in the original contract is not carried over.

Tools Used:

• Manual analysis

Recommendation:

Avoid using constructors for setting crucial contract parameters, instead use the initializer function to set necessary values.

L03: Auction parameters can be changed during an auction

Severity:

• Low Risk

Relevant GitHub Links:

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

Summary:

The AuctionHouse contract is responsible for minting top-voted VerbsTokens and initiating auctions for them. Participants can bid on these auctions, and the highest bidder receives the art piece. The auction proceeds are then divided between the creators of the art piece and the owner of the auction contract.

function settleCurrentAndCreateNewAuction() external override nonReentrant whenNotPaused {
        _settleAuction();
        _createAuction();
    }

An auction is concluded by invoking the settleCurrentAndCreateNewAuction function after the condition “block.timestamp >= _auction.endTime” is met. The _settleAuction function is responsible for transferring the art piece and distributing payments if all conditions are fulfilled. One such condition is that the contract's balance must be greater than the reserve price, signifying that a valid bid has been placed.

function _settleAuction() internal {
        IAuctionHouse.Auction memory _auction = auction;

        require(_auction.startTime != 0, "Auction hasn't begun");
        require(!_auction.settled, "Auction has already been settled");
        //slither-disable-next-line timestamp
        require(block.timestamp >= _auction.endTime, "Auction hasn't completed");

        auction.settled = true;

        uint256 creatorTokensEmitted = 0;
        // 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);
        } else {
            ...
    }

However, as it stands the contract owner can change the reserve price at any time using the setReservePrice function. This allows them to potentially block the highest bidder from winning by setting the reserve price higher than the contract's balance, leading to the burning of the piece and refunding the bidder.

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

        emit AuctionReservePriceUpdated(_reservePrice);
    }

Impact:

Severity: Low. While this ability to change the reserve price mid-auction could be misused to unfairly influence auction outcomes, its impact is limited given the owner address is a trusted role.

Tools Used:

• Manual analysis

Recommendation:

To enhance security, the trusted address in the contract, particularly for sensitive functions like setting auction parameters, should be managed through a multi-signature wallet. This approach ensures that important decisions require consensus among multiple trusted parties, reducing the risk of unauthorized changes due to a compromised account.

L04: Revert state in auction due to computeTotalReward function failure

Severity:

• Low Risk

Relevant GitHub Links:

• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L336
• https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L40

Summary:

The _settleAuction function in the AuctionHouse contract is responsible for finalizing auctions, transferring art pieces to the highest bidder, and paying out the creator and owner. The function has implemented a fail safe method by pausing the contract when the _settleAuction function reverts.

Vulnerability Details:

The auction is concluded using the settleCurrentAndCreateNewAuction function, which is triggered after the condition “block.timestamp >= _auction.endTime” is met. Within this process, the _settleAuction function handles the transfer of the art piece and the distribution of payments.

function _settleAuction() internal {
        ...
                //Buy token from ERC20TokenEmitter for all the creators
                if (creatorsShare > ethPaidToCreators) {
                    creatorTokensEmitted = erc20TokenEmitter.buyToken{value: creatorsShare - ethPaidToCreators}(
                        vrgdaReceivers,
                        vrgdaSplits,
                        IERC20TokenEmitter.ProtocolRewardAddresses({
                            builder: address(0),
                            purchaseReferral: address(0),
                            deployer: deployer
			...
    }

The buyToken function is internally called, it purchases tokens for creators and the buyer. This function includes a step where 2.5% of the purchase is allocated as rewards, calculated by the computeTotalReward function.

function buyToken(
        ...
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        ...
        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );
        ...
    }

This function is designed with specific constraints: if the paymentAmountWei falls below the minimum threshold (0.0000001 ether) or exceeds the maximum limit (50,000 ether), the function will trigger a revert. This can lead to potential issues in certain transaction scenarios.

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

While a direct call to buyToken simply reverts the transaction, a call through _settleAuction can lead to the auction being locked until the auction's split percentages are adjusted or the tokens are burnt.

Impact:

Locking of Auctions: This issue can result in auctions being locked and unable to proceed, requiring manual intervention to resolve

Tools Used:

• Manual analysis

Recommendation:

Implement a fallback mechanism in the _settleAuction function to handle amounts outside the desired range.