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

3 stars 2 forks source link

A MALICIOUS CREATOR OF AN AUCTIONED ART PIECE CAN DoS THE `AuctionHouse._settleAuction` TRANSACTION THUS `DoS` THE SETTLEMENT OF THE CURRENT AUCTION AND CREATION OF A NEW AUCTION #676

Closed c4-bot-2 closed 9 months ago

c4-bot-2 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L384-L395 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L426-L430

Vulnerability details

Impact

The AuctionHouse._settleAuction function is used to settle an auction by finalizing the bid and paying out to the owner and creators.

The payment transfer to each of the creator of the artPiece is executed as shown below:

            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;

                    //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
                    uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
                    ethPaidToCreators += paymentAmount;

                    //Transfer creator's share to the creator
                    _safeTransferETHWithFallback(creator.creator, paymentAmount);
                }
            }

The payment transfer is carried out by calling the _safeTransferETHWithFallback function which uses the low-level call function to transfer the payment ETH amount as shown below:

    assembly {
        // Transfer ETH to the recipient
        // Limit the call to 50,000 gas
        success := call(50000, _to, _amount, 0, 0, 0, 0)
    }

The issue here is that a single malicious creator of the auctioned artPiece can implement a receive function in its smart contract and consume (drain) the entire gas of the transaction (gas bomb can be implemented by a malicious creator) thus reverting the settleAuction transaction. Furthermore this will make the _settleAuction transaction very costly due to draining of gas thus prompting loss of funds to the caller.

As a result the current auction will never be able to be settled. Because it gets reverted since one of the malicious creators of the auctioned artPiece consumes all the gas of the transaction in its receive function.

As a result a new auction can not be created via the AuctionHouse.settleCurrentAndCreateNewAuction function since it initially attempts to settle the current auction but it gets reverted as explained earlier.

As a result the AuctionHouse contract gets DoS for creation and executing new auctions and bids for the Verbs.

Proof of Concept

                    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;

                        //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
                        uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
                        ethPaidToCreators += paymentAmount;

                        //Transfer creator's share to the creator
                        _safeTransferETHWithFallback(creator.creator, paymentAmount);
                    }

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

        assembly {
            // Transfer ETH to the recipient
            // Limit the call to 50,000 gas
            success := call(50000, _to, _amount, 0, 0, 0, 0)
        }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to define a mapping which stores the creator's address and the respective payment amounts of each of the creator, of the auctioned artPiece, and then implement a function claimCreatorPayment callable by each of the creators so they can receive their payment amounts.

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

MarioPoneder commented 9 months ago

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

c4-judge commented 9 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 duplicate of #93

c4-judge commented 9 months ago

MarioPoneder marked the issue as satisfactory