search

code-423n4 / 2023-12-revolutionprotocol-findings

3 stars 2 forks source link

Since art pieces' size is not limited, attacker may block AuctionHouse from creating and settling auctions #178

Open c4-bot-6 opened 10 months ago

c4-bot-6 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L370-L371 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L385 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L209-L238 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L313 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L178 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L282 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L296-L308

Vulnerability details

Note: there is another bug (calling verbs.getArtPieceById in a loop in _settleAuction), but this issue focuses on a different attack vector for creating DoS attack, so in this submission, I assume that the bug is fixed (that is, verbs.getArtPieceById is not called in a loop, but cached before it).

tl;dr

This submission shows two implications of the same bug - not limiting length of pieces of art in CultureIndex::createPiece:

Detailed description

CultureIndex::createPiece allows any user to create art pieces. The only validation regarding art piece size performed there is the validation in validateMediaType which only ensures that relevant field in art piece is non-zero.

If an art piece wins voting, its data is then fetched several times (3 - see note at the beginning of this submission) when verbs.getArtPieceById is called.

If malicious creator creates some nice art piece (for instance of type IMAGE) and hides a very long string in another, irrelevant field (such as ArtPieceMetadata.text) and his piece wins the voting, it will cost a lot of gas to fetch its data both in _settleAuction and _createAuction.

The following implications may occur:

  1. It may either cause a user who calls _settleAuction to pay a lot for gas, or possibly even DoS the entire AuctionHouse.
  2. _createAuction may be DoSed, and, as a result, the entire AuctionHouse may be DoSed, at least until another piece of art wins voting.

Impact

  1. Users may pay very high fees for gas (see the exact gas amounts in the PoC section). In the worst case AuctionHouse will be DoSed as _settleAuction will attempt to consume more than the block gas limit (30 000 000). According to the calculations I have made (see PoC), it's currently possible to cause _settleAuction to use up to ~22 000 000 gas, which is currently less than the block gas limit of 30 000 000. However, operations costs were changing in the past - for example the cost of sload (that is used when art piece's data is fetched) increased from 50 to 2100 (or 100 in case of warm access) from the frontier hardfork (see frontier and shanghai), so it is possible that after another hardfork, it will be possible to DoS AuctionHouse using this exploit. Moreover, it is possible that block gas limit will decrease, which will cause the same effect.
  2. _createAuction will reach block gas limit, which will make creating new auctions impossible, until another art piece wins voting (but the "malicious" art piece will still participate in subsequent votings).

Both exploits require a malicious artwork to win the voting, however, as I have mentioned before, the attacker can hide a very long string in an unrelated field of ArtPieceMetadata structure, so that users may not even notice this and will only pay attention for the image contained in that artwork.

Alternatively, attacker can buy some NontransferableERC20Votes at the beginning and win the voting by himself, before quorum becomes too big.

The impact is severe, but the attack is also costly for the attacker, and under normal circumstances, I would submit the issue as Medium. However, the exploit presented in this submission breaks two invariants that, according to the Sponsor " should NEVER EVER be broken":

This is the reason, I'm submitting the issue as High.

Proof of Concept

The test I'm showing will simulate a scenario when a malicious user creates a piece of art containing very long data, that piece wins voting and is then sold.

I assume that the attacker doesn't perform any additional exploits and the bug that I mentioned at the very beginning of this submission is fixed. Moreover, I assume that the attacker specifies only one creator account (if he specified more, he could cause DoS with a less cost - as I have shown in another submission, just by specifying malicious creators he can cause _settleAuction to consume additional ~20 000 000 of gas) as I wanted to measure only the impact of a "heavy" NFT on the gas cost.

The exploit requires a very long string to be passed - for the exploit to be the most destructive, the string would have to be ~1.3MB long (this will achieve 30 000 000 for the attacker and ~22 000 000 for _settleAuction. The string was not explicitly put in the PoC in order to keep this submission smaller - in order to test it, please add a very long string of \x00's in the place tagged with <- put here a very long string [...]. It is necessary for the string to only contain \x00's since it will significantly reduce cost for the attacker (as sreset, that cost 2900 gas will be used instead of sset, which costs 20 000).

The test will output 3 important values:

Please put the following test into AuctionSettling.t.sol and run it. Please remember to add a very long string of \x00's in the relevant place:

// create an auction and finish it
    function _createAndFinishAuction() internal returns(uint)
    {
        uint nCreators = 1;
        address[] memory creatorAddresses = new address[](nCreators);
        uint256[] memory creatorBps = new uint256[](nCreators);
        uint256 totalBps = 0;
        address[] memory creators = new address[](nCreators + 1);
        for (uint i = 0; i < nCreators + 1; i++)
        {
            creators[i] = address(uint160(0x1234 + i));
        }

        for (uint256 i = 0; i < nCreators; i++) {
            creatorAddresses[i] = address(creators[i]);
            if (i == nCreators - 1) {
                creatorBps[i] = 10_000 - totalBps;
            } else {
                creatorBps[i] = (10_000) / (nCreators - 1);
            }
            totalBps += creatorBps[i];
        }

        uint gas1 = gasleft();
        uint256 verbId = createArtPieceMultiCreator(
            "Multi Creator Art",
            "An art piece with multiple creators", 
            ICultureIndex.MediaType.IMAGE,
            "ipfs://multi-creator-art",
            "",
            "\x00\x00\x00\x00\x00\x00\x00", // <- put here a very long string (~1.3 MB for _settleAuction to reach 22 000 000 of gas, and ~900 KB for _createAuction to reach block gas limit)
            creatorAddresses,
            creatorBps
        );
        uint gas2 = gasleft();

        vm.startPrank(auction.owner());
        uint gas3 = gasleft();
        auction.unpause();
        uint gas4 = gasleft();
        console.log("gas used for creating auction:", gas3 - gas4);
        vm.stopPrank();

        uint256 bidAmount = auction.reservePrice();
        vm.deal(address(creators[nCreators]), bidAmount + 1 ether);
        vm.startPrank(address(creators[nCreators]));
        auction.createBid{ value: bidAmount }(verbId, address(creators[nCreators]));
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        // return amount of gas spent by the attacker on creating a piece of art
        return gas1 - gas2;
    }

    function testDOS2() public
    {
        uint gasConsumption1;
        uint gasConsumption2;
        uint gas0;
        uint gas1;

        vm.startPrank(cultureIndex.owner());
        cultureIndex._setQuorumVotesBPS(0);
        vm.stopPrank();

        gasConsumption1 = _createAndFinishAuction();

        gas0 = gasleft();
        auction.settleCurrentAndCreateNewAuction();
        gas1 = gasleft();

        gasConsumption2 = gas0 - gas1;
        console.log("gas used by the attacker: ", gasConsumption1);
        console.log("gas used in settleCurrentAndCreateNewAuction:",gasConsumption2);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Limit the length of all fields in ArtPieceMetadata.

Assessed type

DoS

c4-pre-sort commented 10 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #93

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #195

c4-judge commented 10 months ago

MarioPoneder changed the severity to 2 (Med Risk)

MarioPoneder commented 10 months ago

See comment on primary issue: https://github.com/code-423n4/2023-12-revolutionprotocol-findings/issues/195#issuecomment-1879684718

c4-judge commented 10 months ago

MarioPoneder marked the issue as partial-25

c4-judge commented 9 months ago

MarioPoneder marked the issue as not a duplicate

c4-judge commented 9 months ago

MarioPoneder marked the issue as satisfactory

c4-judge commented 9 months ago

MarioPoneder marked the issue as selected for report

c4-judge commented 9 months ago

MarioPoneder marked the issue as primary issue