Revolution Protocol - ptsanev's results

Lines of code

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

Vulnerability details

Impact

Changing the reserve price variable while an auction is running or near an auction's end could cause the winner to unfairly lose his NFT.

Proof of Concept

Whenever the auction time runs out and _settleAuction is invoked, we make a check to see the current ETH balance of the contract against the reserve price. If everything was done correctly, the ETH balance would be >= reserve price and the NFT would be minted to the winner. Otherwise, these was no bidder and the NFT is burned. But in the scenario where the bid is less than the reserve price, the ETH gets refunded and the NFT burned. Such scenario at first seems unlikely, but factors like network congestion, front-running and block-stuffing could (highly unlikely) result in such a scenario where there was low auction activity so the only bidder bid the reserve price, an admin decides to increase the reserve price, thus leading to the winner not receiving his NFT.

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure the reserve price can only be called during an auction and not during it's settlement or even better - only between auctions as to not serve an unfair advantage for the earlier bidders who would have a different entry.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L234 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L498-L503

Vulnerability details

Impact

The quorum votes property are a safeguard for the protocol to prevent flash attacks on art pieces. They are calculated at the time of the art piece creation and are saved in it's struct, effectively meaning that older art pieces would either have an unfair advantage or disadvantage over newer art pieces in case the admin decides to do _setQuorumVotesBPS() in times of low or high activity.

Proof of Concept

In the implementation of the createPiece we can see that quorum votes calculation happens at the time of creation, meaning it is constant throughout. In times of bad network, low/too high protocol activity, etc. the team has the option to change the quorum votes BPS needed to pass and let a piece be dropped. This creates an unfair advantage/disadvantage for older pieces, since they quorum is already calculated and saved using the old value, instead of at the time of dropping.

Scenario 1: There is low protocol activity, so the current quorum proves too difficult to pass and to avoid a system DoS, the threshold gets reduced. Now all of the old art pieces would still be checked against the old quorum, newer art pieces (if any in times of low activity), would have an unfair advantage, which isn't even a guarantee they would get most votes, meaning a DoS could happen anyway if an older pieces gets stuck in the top of the maxHeap

Scenario 2: Activity is high and the protocol deems it reasonable to increase the quorum to avoid potential flash attacks for rigging. Now older pieces are checked against the old value, deeming them much more favorable to pass quorum and unnecessarily favoring them over newer pieces. A user might even consider front-running the quorum upgrade, since the latest added piece would have 1. Lower quorum threshold 2. More token holders who can vote for it (because of how the snapshots work)

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of doing newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; at the time of creation, calculate the quorum threshold only at the time of dropping. So instead of require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes) you should have require(totalVoteWeights[piece.pieceId] >= (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000 to account for the updated quorumVotesBPS.

Assessed type

Context