Revolution Protocol - nmirchev8's results

Lines of code

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

Vulnerability details

Impact

• MaxHeap.sol has a crucial role for the whole protocol as it stores and filters the voting for the pieces and it is very important to work correctly
• However if we take a look at extractMax() we can notice something very suspicious:
• • We remove the item with maximum value (first one in the heap), we set the last item on his place and decrement size. We can notice that we actually haven't removed the last item from his previous place inside heap and now we store the same piece id on two places, which is wrong. Note that: major imapct that comes from this root would araise if we call updateValue with new value, which is smaller than the previous, which would be the case if the protocol has functionality to downvote, which they play to implement in future. However the issue is regarding a data structure implementation, which is whole in the scope of the audit and it is confirmed and valuable for the sponsor:

Proof of Concept

I have created detailed diagram showing the state of the heap after each operation:

Here is a coded PoC with detailed comments. You can paste it inside /packages/revolution/test/max-heap/Updates.t.sol , import import "forge-std/console.sol"; at the beginning of the file and run: FOUNDRY_PROFILE=dev forge test --match-test "testUpdateValueExtractMaxDoesntEraseLastValue" -vv at ./revolution directory

Tools Used

Manual Review

• When extracting max value, you should delete the duplicated piece:

    function extractMax() external onlyAdmin returns (uint256, uint256) {
        require(size > 0, "Heap is empty");

        uint256 popped = heap[0];
        heap[0] = heap[--size];
+       delete heap[size]
+       positionMapping[heap[0]] = 0
        maxHeapify(0);

        return (popped, valueMapping[popped]);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Details:

We can see that when a pice is created there is a validation for the provided mediaType, but we should note that ArtPieceMetadata has multiple string fields, which are never checked:

    struct ArtPieceMetadata {
        string name;
        string description;
        MediaType mediaType;
        string image;
        string text;
        string animationUrl;
    }

On a later stage (if a piece has reached quorum and is most voted) all those values are copied inside VerbsToken. This operation is crucial, because it is part of the flow to close an auction and start another, which is getting the most voted piece. It is possible for a user maliciously or not to put a large string data when creating a piece, so later the fees are extremely high and unpayable. In the PoC section I have tested with a large text for description, which is 580 words (A normal amount if someone want to include a narrative or story to his art). In this case gas spent for only opening the auction is 3371079, which for Ethereum Mainnet would cost:60 (gwei) * 3371079 = 202264740 ~ $400, which is a lot and can be a lot larger, because currently there is no validation. Note that also in testing environment we have an empty heap, which in real application with some time functionating would add more operations , as extracting and ordering heap is part of the flow

Impact

• Temporary stucked state, because noone wants to pay high. It is not excluded a larger attack, which would fill the whole block and will always revert when executing the minting flow.
• Unhappy users
• Bad reputation in case of such scenario

Proof of Concept

Here is a simple test, which you can run in any of files inside packages/revolution/test/auction using FOUNDRY_PROFILE=dev forge test --match-test "testGasCostForLargeDescription" -vv when you are inside ./revolution

Tools Used

Manual Review

Recommended Mitigation Steps

• Add a validation for the metadata fields to reduce unpleasant consequences
• Example:

uint256 public constant MAX_METADATA_FIELDS_L = 1000;

function validateMetadata(ArtPieceMetadata calldata metadata) internal pure {
      	uint256 allFieldsLength = metadata.name.length + metadata.description.length + metadata.image.length + metadata.text.length + metadata.animationUrl.length;
        require( allFieldsLength <= MAX_METADATA_FIELDS_L);

    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L41

Vulnerability details

Impact

• There are many parameters in the system, which prevent this scenario, but neither of them is guaranteed and has a check for min/max amount
• The problem here is that in specific case, where reservePrice and winning bid are close to zero and entropyRateBps is high enough, settling an Auction can hit INVALID_ETH_AMOUNT and block new auction from starting.
• The func flow is _settleAuction => buyToken => _handleRewardsAndGetValueToSend => computeTotalReward

Proof of Concept

Here is a gist with PoC. In a comment above the test I have provided the params for the configuration, which allow this revert to happen https://gist.github.com/NicolaMirchev/59e55575a166b89a78c0cc01b67c8aea

Tools Used

Manual Review

Recommended Mitigation Steps

• You can set constant for minimum reservePrice to 0.001 ether
• You can add check before buyTokens() inside _settleAuction:

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Summary

Observing the main and most important flow of revolution protocol (mint the winning NFT and start auction) we can immediately see the complexity of the nested functions and actions. Since we work in blockchain gas cost and execution is crucial as it costs money and in some situations it could result in DoS. Currently the flow consist of settling an auction, transferring rewards, minting erc20votes, extracting value from constantly increasing heap and minting new NFT with arbitrary string data. All this should happen in one transaction initiated inside settleCurrentAndCreateNewAuction. But the worst part is the large tolerance of untrusted addresses, which are called, forwarding 50_000 gas.

Details

Lets carefully observe the flow of settling an auction:

• _settleAuction function would iterate over the verb creators and call
• We can note that creators limit is set to 100
• So worst case scenario, which is a valid and real one is to have 100 calls, each consuming 50_000 gas = 5_000_000 gas only for transffering rewards, which for current state of mainnet (gas = 45 gwei; ETH = $2250) is ~ 0.225 ETH = ~$500 in gas only for a couple of lines of this complext function flow.
• In the nested function ERC20TokenEmitter::buyTokens, which is also with great complexity, we again iterate over the creators array.

Impact

• The impact is big, because griefing creators contract may go unnoticed and reach quorum because of the good art this malicious actor has provided. This is the enough to reach a DoS on Ethereum Mainnet, which a sponsor has confirmed that is supported network Mainnet included
• This will stop the possibility to mint new nfts and start auction, which could result in bad reputation and loss of participants

Proof of Concept

I have coded a PoC, where you can examine that the issue is actually bigger, as the whole flow could exceeded Mainnet gas limit, which would result in DoS. Even if admin pause and call only settleAuction, the gas used for the flow is > 30_000_000 which is mainned average gas limit.

• To run the test paste the provided MaliciousParticipant.sol inside packages/revolution/test/auction/AuctionSettling.t.sol
• add the provided test function inside the test contract
• navigate to root foundry directory (./revolution)
• run FOUNDRY_PROFILE=dev forge test --match-test "testSpendGasAmountOnLargeCreatorsArray" -vv for faster execution

Tools Used

Manual Review

Recommended Mitigation Steps

• A maximum of 5000 gas should be enough for ETH transffer purpose
• Consider a way to reduce the complexity of the flow and nested operations. Maybe calculating and minting ERC20 votes should happen in another transaction and an amount from reward would go for the gas, because even for 5 greifing creators accounts (which could be one account repeated 5 times) the spent for the flow is 1_000_000 gas = ~ $100 on Mainnet.

Assessed type

DoS