Revolution Protocol - mojito_auditor's results

Lines of code

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

Vulnerability details

Impact

In the AuctionHouse contract, a reservePrice limit is set for the final price of any auction. If the last bid does not meet this reservePrice, the auction is considered unsuccessful and the Verb token is burnt. This behavior is checked and executed in the function _settleAuction().

// Check if contract balance is greater than reserve price
// @audit this could by bypass by transferring ETH to this contract directly using self-destruct
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);
}

However, the check uses the current contract balance, which could be easily bypassed by transferring ETH directly to the contract using self-destruct.

Proof of Concept

Even though the AuctionHouse does not have an external receive function, an attacker can still use self-destruct on another contract and refund the ETH to the AuctionHouse contract. More information about self-destruct can be found at:

https://solidity-by-example.org/hacks/self-destruct/

Tools Used

Manual Review

Recommended Mitigation Steps

Consider checking the reservePrice against the last bid instead of the current contract balance.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In CultureIndex, the createPiece() function records the current vote and ERC20 supplies. It also sets newPiece.creationBlock = block.number.

newPiece.totalVotesSupply = _calculateVoteWeight( 
    erc20VotingToken.totalSupply(),
    erc721VotingToken.totalSupply()
);
newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
newPiece.metadata = metadata;
newPiece.sponsor = msg.sender;
newPiece.creationBlock = block.number;

When voting for a piece, the voting power is obtained at the creationBlock. However, this may cause a mismatch between totalVotesSupply and totalERC20Supply. This is because additional transactions within the same block as createPiece() could alter the ERC20Supply.

Proof of Concept

Consider this scenario:

1. At block 100, createPiece() is called with erc20VotingToken.totalSupply() = 1000. This results in totalERC20Supply = 1000.
2. In the same block 100, after the createPiece() transaction, another transaction buyToken() is made to buy a large amount of voting token, which will increase erc20VotingToken.totalSupply() = 5000.
3. Even though these tokens were bought after the piece was created, they still have voting power because the block number is still 100. This also makes totalERC20Supply and totalVotesSupply incorrect. Although voting supply and ERC20 supply are calculated with erc20VotingToken.totalSupply() = 1000, the actual ERC20 supply that is allowed to vote will be 5000.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider getting the voting power at creationBlock - 1 instead when casting a vote for an art piece.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the MaxHeap contract, the maxHeapify() function is used to reheapify the heap to maintain the heap property. Starting from a given position in the heap, it gets the values of the left and right children. If a child's value is greater than the current position's value, they are swapped to maintain the heap's structure.

function maxHeapify(uint256 pos) internal {
    uint256 left = 2 * pos + 1;
    uint256 right = 2 * pos + 2;

    uint256 posValue = valueMapping[heap[pos]];
    uint256 leftValue = valueMapping[heap[left]];
    uint256 rightValue = valueMapping[heap[right]];

    if (pos >= (size / 2) && pos <= size) return; 

    // @audit not check if `right` node is exist or not (could be a node has been removed earlier)
    if (posValue < leftValue || posValue < rightValue) { 
        if (leftValue > rightValue) {
            swap(pos, left);
            maxHeapify(left);
        } else {
            swap(pos, right);
            maxHeapify(right);
        }
    }
}

The problem is that there may be cases where the right child does not exist but contains the value of a previously removed node. If the removed node has a larger value than the current node, the removed node will be swapped back into the heap. This breaks the heap's structure and could return the incorrect maximum value when queried.

Proof of Concept

Please copy this test to Updates.t.sol and also import console.sol.

function testMaxHeapify() public {
    // Modified from testUpdateValueDecrease()
    maxHeapTester.insert(1, 20);
    maxHeapTester.insert(2, 20);
    maxHeapTester.insert(3, 20);

    maxHeapTester.extractMax();
    maxHeapTester.extractMax();
    maxHeapTester.extractMax();
  
    // At this moment, the heap is empty.

    maxHeapTester.insert(4, 10);
    maxHeapTester.insert(5, 10);

    // Trigger maxHeapify()
    maxHeapTester.updateValue(4, 7);

    (uint256 itemId, uint256 value) = maxHeapTester.getMax();

    // Output
    // Item ID:  3
    // Value:  20
    console.log("Item ID: ", itemId);
    console.log("Value: ", value);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Check if the right node exists before making comparisons.

Assessed type

Other