Revolution Protocol - mahdirostami's results

Lines of code

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

Vulnerability details

Description

In the AuctionHouse::_settleAuction function of the AuctionHouse contract, there is a check that compares the contract's balance (address(this).balance) with the reserve price. If the contract balance is less than the reserve price, the NFT is intended to be burned. However, the check is susceptible to exploitation, as it relies solely on the contract's balance.

        // Check if contract balance is greater than reserve price
@>        if (address(this).balance < reservePrice) { //@audit bidder could send eth
            // 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);

If the owner increase the AuctionHouse::reservePrice, The issue arises when a bidder sent less than the reserve price, but later sends additional ETH to the contract, potentially bypassing the reserve price check and resulting in less value being sent to the auction creator and owner.

Impact

This behavior could lead to less value being distributed to both the auction creator and the owner. While the bidder doesn't gain any extra value, it affects the shares sent to the creator and owner. It also impact further auctions as this ETH will remain in contract.

Impact: Less share sent to both the auction creator and owner. Likelihood: Depending on the increase in AuctionHouse::reservePrice. Feasibility: Feasible, as it requires sending additional ETH to the contract after the initial bid (using selfdestruct).

Recommended Mitigation

Two mitigation approaches are suggested:

1. Use an internal state instead of contract.balance for the reserve price check
2. Use contract.balance for splitting shares instead of _auction.amount.

Tools Used

Manual Review

Proof of Concept (Proof of Code)

A test scenario is provided to illustrate the potential issue:

    function test_donation() public{
        createDefaultArtPiece();
        auction.unpause();
        console2.log(address(auction).balance);
        vm.deal(mahdi, 10 ether);
        vm.startPrank(mahdi);
        auction.createBid{ value: 1 ether }(0, mahdi);
        console2.log("reservePrice:", auction.reservePrice());
        console2.log("contract balance:", address(auction).balance);
        vm.startPrank(auction.owner());
        console2.log("reservePrice changed from 1 eth to 2 eth");
        auction.setReservePrice(2 ether); 
        auction.pause();
        vm.stopPrank();
        //go in future
        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        //user send some eth to contract to pass test, by selfdestruct or ...
        vm.deal(address(auction), 2 ether);
        console2.log("user sent some eth to pass reserve price check, contract balance:", address(auction).balance);
        auction.settleAuction();
        console2.log("contract balance after settle function:", address(auction).balance);
        console2.log(auction.verbs().balanceOf(mahdi));
    }

Logs:

Running 1 test for test/auction/AuctionSettling.t.sol:AuctionHouseSettleTest
[PASS] test_donation() (gas: 1493067)
Logs:
  0
  reservePrice: 1000000000000000000
  contract balance: 1000000000000000000
  reservePrice changed from 1 eth to 2 eth
  user sent some eth to pass reserve price check, contract balance: 2000000000000000000
  contract balance after settle function: 1000000000000000000
  1

This test scenario demonstrates a potential situation where a user could send additional ETH to the contract, leading to unexpected behavior in the settlement process.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

When attempting to drop the top-voted piece, a check is performed to ensure it meets the required quorum votes. This check is implemented using the piece.quorumVotes:

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

The piece.quorumVotes is calculated during creation time based on the formula:

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

Capturing totalVotesSupply during creation time is necessary to ensure fairness among different pieces created at different times. However, computing newPiece.quorumVotes at creation time may introduce potential unfairness. The problem comes up when the owner change the CultureIndex::quorumVotesBPS, making piece.quorumVotes not fair for different pieces. Sometimes, the owner might want to change CultureIndex::quorumVotesBPS, like making it less if it's too much. After making it less, new pieces could more easily meet the quorum votes check.

Impact

If the owner increases CultureIndex::quorumVotesBPS, it could introduce unfairness against subsequently created pieces. Conversely, decreasing it could result in the opposite effect. The likelihood depends on the owner's decision to change CultureIndex::quorumVotesBPS. Whenever the owner changes its value, unfairness may arise

Recommended Mitigation

To address this issue, catching totalERC20Supply is sufficient to differentiate between pieces created at different times. The problem can be resolved by not calculating newPiece.quorumVotes at creation time and instead calculating it at the time of dropping. Here's the suggested modification:

         require(msg.sender == dropperAdmin, "Only dropper can drop pieces");
 
         ICultureIndex.ArtPiece memory piece = getTopVotedPiece();
-        require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped.");
+        require(totalVoteWeights[piece.pieceId] >= (quorumVotesBPS * piece.totalVotesSupply) / 10_000, "Does not meet quorum votes to be dropped.");

This modification ensures there is no difference between pieces after changing CultureIndex::quorumVotesBPS.

Tools Used

Manual Review

Proof of Concept (Proof of Code)

The following test demonstrates the impact. Increasing totalSupply and quorumVotesBPS shows unfairness before and after changing quorumVotesBPS:

    function test_test() public {
        uint256 erc20Supply = 1e18;
        uint256 quorumVotesBPS = 1_000;
        uint256 secondQuorumVotesBPS = 2_000;

        // Set the quorum BPS
        cultureIndex._setQuorumVotesBPS(quorumVotesBPS);

        cultureIndex.transferOwnership(address(erc721Token));
        vm.startPrank(address(erc721Token));
        cultureIndex.acceptOwnership();

        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(address(this), erc20Supply);

        // Create First art pieces
        uint256 pieceId = createDefaultArtPiece();

        // Mint token and Increase the quorum BPS 
        erc20Token.mint(address(this), erc20Supply);
        vm.startPrank(address(erc721Token));
        cultureIndex._setQuorumVotesBPS(secondQuorumVotesBPS);

        // Create Second art pieces
        uint256 pieceId2 = createDefaultArtPiece();

        CultureIndex.ArtPiece memory piece = cultureIndex.getPieceById(pieceId);
        CultureIndex.ArtPiece memory piece2 = cultureIndex.getPieceById(pieceId2);


        // Values
        console2.log("totoalSupply in piece 1 ", piece.totalERC20Supply);
        console2.log("totoalSupply in piece 2 ", piece2.totalERC20Supply);
        console2.log("quorumVotes in piece 1 ", piece.quorumVotes);
        console2.log("quorumVotes in piece 2 ", piece2.quorumVotes);
    }

Logs:

@collectivexyz/revolution:test: [PASS] test_test() (gas: 1029724)
@collectivexyz/revolution:test: Logs:
@collectivexyz/revolution:test:   totoalSupply in piece 1  1000000000000000000
@collectivexyz/revolution:test:   totoalSupply in piece 2  2000000000000000000
@collectivexyz/revolution:test:   quorumVotes in piece 1  100000000000000000
@collectivexyz/revolution:test:   quorumVotes in piece 2  400000000000000000

As shown above, increasing quorumVotesBPS creates fairness between pieces. For piece 1, 10 percent of votes depend on total supply at creation time, and for piece 2, 20 percent is needed.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L400 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L165 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L40 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L23-L24

Vulnerability details

Description

In the AuctionHouse::_settleAuction function of the AuctionHouse contract, there is a section where tokens are bought from ERC20TokenEmitter for all the creators. The relevant code is as follows:

                //Buy token from ERC20TokenEmitter for all the creators
                if (creatorsShare > ethPaidToCreators) { 
@>                    creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(//@audit check for minPurchaseAmount and maxPurchaseAmount
                        vrgdaReceivers,
                        vrgdaSplits,
                        IERC20TokenEmitter.ProtocolRewardAddresses({
                            builder: address(0),
                            purchaseReferral: address(0),
                            deployer: deployer
                        })
                    );
                }

The buyToken function in erc20TokenEmitter involves calling TokenEmitterRewards::_handleRewardsAndGetValueToSend. Inside _handleRewardsAndGetValueToSend, there's a call to RewardSplits::computeTotalReward, which includes a check for msg.value.

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

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

The computeTotalReward function in RewardSplits has a check for msg.value constraints.

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

which could cause DOS in AuctionHouse::_settleAuction function. after that the owner of AuctionHouse must change AuctionHouse::entropyRateBps or change AuctionHouse::creatorRateBps to mitigate DOS.

Impact

The impact is a potential denial-of-service (DoS) in the AuctionHouse::_settleAuction function.

impact and likelihood, feasibility. The impact will be DOS in AuctionHouse::_settleAuction. Likelihood: Low, as it requires a specific _auction.amount that, after giving creator and owner shares, falls into a specific range.

Recommended Mitigation

Check for the specified conditions in the AuctionHouse::_settleAuction function and, if applicable, split these shares accordingly. Ensure that minPurchaseAmount and maxPurchaseAmount are considered to prevent DoS scenarios.

Tools Used

Manual Review

Assessed type

DoS