OOG Auction Coverage increase + final audit cleanups (#96)

* further test * minor cleanup * clean up test case here * 30k gas instead * fix * 1 less call to getArtPieceById * nvm * require reserve price > 0 * minor gas savings in maxheap * var clarity * fix heap tests * leave public for now * don't take 40 minutes to test * Create slow-seahorses-tap.md * rm logs from tests * try this * cmon * reduce fuzz runs for this heavy func * fix * comment updatres * be specific about numbers
collectivexyz · Jan 29, 2024 · 5b94905 · 5b94905
1 parent ca5633c
commit 5b94905
Show file tree

Hide file tree

Showing 26 changed files with 212 additions and 203 deletions.
diff --git a/.changeset/slow-seahorses-tap.md b/.changeset/slow-seahorses-tap.md
@@ -0,0 +1,5 @@
+---
+"@cobuild/revolution": patch
+---
+
+readability improvements
diff --git a/packages/protocol-rewards/test/utils/PointsEmitterLibrary.sol b/packages/protocol-rewards/test/utils/PointsEmitterLibrary.sol
@@ -1525,15 +1525,15 @@ contract RevolutionPointsEmitter is
         BuyTokenPaymentShares memory buyTokenPaymentShares = _calculateBuyTokenPaymentShares(msgValueRemaining);
 
         // Calculate tokens to emit to creators
-        int totalTokensForCreators = buyTokenPaymentShares.creatorsGovernancePayment > 0
+        int256 totalTokensForCreators = buyTokenPaymentShares.creatorsGovernancePayment > 0
             ? getTokenQuoteForEther(buyTokenPaymentShares.creatorsGovernancePayment)
             : int(0);
 
         // Update total tokens emitted for this purchase with tokens for creators
         if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;
 
         // Tokens to emit to buyers
-        int totalTokensForBuyers = buyTokenPaymentShares.buyersShare > 0
+        int256 totalTokensForBuyers = buyTokenPaymentShares.buyersShare > 0
             ? getTokenQuoteForEther(buyTokenPaymentShares.buyersShare)
             : int(0);
 

diff --git a/packages/revolution/src/AuctionHouse.sol b/packages/revolution/src/AuctionHouse.sol
@@ -128,6 +128,7 @@ contract AuctionHouse is
         _pause();
 
         if (_auctionParams.creatorRateBps < _auctionParams.minCreatorRateBps) revert CREATOR_RATE_TOO_LOW();
+        if (_auctionParams.reservePrice == 0) revert RESERVE_PRICE_INVALID();
 
         revolutionToken = IRevolutionToken(_revolutionToken);
         revolutionPointsEmitter = IRevolutionPointsEmitter(_revolutionPointsEmitter);
@@ -276,6 +277,8 @@ contract AuctionHouse is
      * @dev Only callable by the owner.
      */
     function setReservePrice(uint256 _reservePrice) external override onlyOwner {
+        if (_reservePrice == 0) revert RESERVE_PRICE_INVALID();
+
         reservePrice = _reservePrice;
 
         emit AuctionReservePriceUpdated(_reservePrice);
@@ -345,19 +348,21 @@ contract AuctionHouse is
 
         // Check if contract balance is greater than reserve price
         if (_auction.amount < reservePrice) {
-            // If contract balance is less than reserve price, refund to the last bidder
+            // If winning bid is less than reserve price, refund to the last bidder
             if (_auction.bidder != address(0)) {
                 _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
             }
 
             // And then burn the token
             revolutionToken.burn(_auction.tokenId);
         } else {
-            //If no one has bid, burn the token
-            if (_auction.bidder == address(0))
+            if (_auction.bidder == address(0)) {
+                //If no one has bid, burn the token
                 revolutionToken.burn(_auction.tokenId);
-                //If someone has bid, transfer the token to the winning bidder
-            else revolutionToken.transferFrom(address(this), _auction.bidder, _auction.tokenId);
+            } else {
+                //If someone has bid and won, transfer the token to the winning bidder
+                revolutionToken.transferFrom(address(this), _auction.bidder, _auction.tokenId);
+            }
 
             if (_auction.amount > 0) {
                 // Ether going to owner of the auction
@@ -373,7 +378,7 @@ contract AuctionHouse is
                 uint256[] memory vrgdaSplits = new uint256[](numCreators);
                 address[] memory vrgdaReceivers = new address[](numCreators);
 
-                //Transfer auction amount to the DAO
+                //Transfer auction amount to the owner
                 _safeTransferETHWithFallback(owner(), auctioneerPayment);
 
                 //Transfer creator's share to the creator, for each creator, and build arrays for revolutionPointsEmitter.buyToken
@@ -384,20 +389,22 @@ contract AuctionHouse is
                         .creators;
 
                     //Calculate the amount to be paid to the creators
-                    uint entropyRateAmount = creatorsShare * entropyRateBps;
+                    uint256 directPayment = creatorsShare * entropyRateBps;
 
                     //If the amount to be spent on governance for creators is less than the minimum purchase amount for points
-                    if ((creatorsShare - (entropyRateAmount / 10_000)) <= revolutionPointsEmitter.minPurchaseAmount()) {
+                    if ((creatorsShare - (directPayment / 10_000)) <= revolutionPointsEmitter.minPurchaseAmount()) {
                         //Set the amount to the full creators share, so creators are paid fully in ETH
-                        entropyRateAmount = creatorsShare * 10_000;
+                        //10_000 assumes 100% in BPS of the creators share is paid to the creators
+                        directPayment = creatorsShare * 10_000;
                     }
 
                     for (uint256 i = 0; i < numCreators; i++) {
                         vrgdaReceivers[i] = creators[i].creator;
                         vrgdaSplits[i] = creators[i].bps;
 
                         //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
-                        uint256 paymentAmount = (entropyRateAmount * creators[i].bps) / (10_000 * 10_000);
+                        //Do division at the end of operations to avoid rounding errors, don't divide before multiplying
+                        uint256 paymentAmount = (directPayment * creators[i].bps) / (10_000 * 10_000);
                         ethPaidToCreators += paymentAmount;
 
                         //Transfer creator's share to the creator
@@ -441,8 +448,8 @@ contract AuctionHouse is
 
         assembly {
             // Transfer ETH to the recipient
-            // Limit the call to 50,000 gas
-            success := call(50000, _to, _amount, 0, 0, 0, 0)
+            // Limit the call to 30,000 gas
+            success := call(30000, _to, _amount, 0, 0, 0, 0)
         }
 
         // If the transfer failed:

diff --git a/packages/revolution/src/RevolutionPointsEmitter.sol b/packages/revolution/src/RevolutionPointsEmitter.sol
@@ -166,7 +166,9 @@ contract RevolutionPointsEmitter is
         uint256 msgValueRemaining
     ) internal view returns (BuyTokenPaymentShares memory buyTokenPaymentShares) {
         // Calculate share of purchase amount reserved for buyers
-        buyTokenPaymentShares.buyersShare = msgValueRemaining - ((msgValueRemaining * creatorRateBps) / 10_000);
+        buyTokenPaymentShares.buyersGovernancePayment =
+            msgValueRemaining -
+            ((msgValueRemaining * creatorRateBps) / 10_000);
 
         // Calculate ether directly sent to creators
         buyTokenPaymentShares.creatorsDirectPayment =
@@ -212,14 +214,14 @@ contract RevolutionPointsEmitter is
         BuyTokenPaymentShares memory buyTokenPaymentShares = _calculateBuyTokenPaymentShares(msgValueRemaining);
 
         // Calculate tokens to emit to creators
-        int totalTokensForCreators = buyTokenPaymentShares.creatorsGovernancePayment > 0
+        int256 totalTokensForCreators = buyTokenPaymentShares.creatorsGovernancePayment > 0
             ? getTokenQuoteForEther(buyTokenPaymentShares.creatorsGovernancePayment)
             : int(0);
 
         // Deposit owner's funds, and eth used to buy creators gov. tokens to owner's account
         _safeTransferETHWithFallback(
             owner(),
-            buyTokenPaymentShares.buyersShare + buyTokenPaymentShares.creatorsGovernancePayment
+            buyTokenPaymentShares.buyersGovernancePayment + buyTokenPaymentShares.creatorsGovernancePayment
         );
 
         // Transfer ETH to creators
@@ -236,9 +238,10 @@ contract RevolutionPointsEmitter is
         uint256 bpsSum = 0;
         uint256 addressesLength = addresses.length;
 
-        // Tokens to emit to buyers - ENSURE we do this after minting to creators, so that the total supply is correct
-        int totalTokensForBuyers = buyTokenPaymentShares.buyersShare > 0
-            ? getTokenQuoteForEther(buyTokenPaymentShares.buyersShare)
+        // Tokens to mint to buyers
+        // ENSURE we do this after minting to creators, so that the total supply is correct
+        int256 totalTokensForBuyers = buyTokenPaymentShares.buyersGovernancePayment > 0
+            ? getTokenQuoteForEther(buyTokenPaymentShares.buyersGovernancePayment)
             : int(0);
 
         //Mint tokens to buyers
@@ -255,7 +258,7 @@ contract RevolutionPointsEmitter is
         emit PurchaseFinalized(
             msg.sender,
             msg.value,
-            buyTokenPaymentShares.buyersShare + buyTokenPaymentShares.creatorsGovernancePayment,
+            buyTokenPaymentShares.buyersGovernancePayment + buyTokenPaymentShares.creatorsGovernancePayment,
             msg.value - msgValueRemaining,
             uint256(totalTokensForBuyers),
             uint256(totalTokensForCreators),

diff --git a/packages/revolution/src/culture-index/MaxHeap.sol b/packages/revolution/src/culture-index/MaxHeap.sol
@@ -63,8 +63,8 @@ contract MaxHeap is IMaxHeap, VersionedContract, UUPS, Ownable2StepUpgradeable,
     /// @notice Reverts for position zero
     error INVALID_POSITION_ZERO();
 
-    /// @notice Reverts for invalid piece ID
-    error INVALID_PIECE_ID();
+    /// @notice Reverts for invalid item ID
+    error INVALID_ITEM_ID();
 
     ///                                                          ///
     ///                         INITIALIZER                      ///
@@ -90,7 +90,7 @@ contract MaxHeap is IMaxHeap, VersionedContract, UUPS, Ownable2StepUpgradeable,
     mapping(uint256 => uint256) public heap;
 
     /// @notice the number of items in the heap
-    uint256 public size = 0;
+    uint256 public size;
 
     /// @notice composite mapping of the heap position (index in the heap) and priority value of a specific item in the heap
     /// To enable value updates and indexing on external itemIds
@@ -99,6 +99,8 @@ contract MaxHeap is IMaxHeap, VersionedContract, UUPS, Ownable2StepUpgradeable,
         uint256 value;
         uint256 heapIndex;
     }
+
+    /// @notice mapping of itemIds to their priority value and heap index
     mapping(uint256 => Item) public items;
 
     /// @notice Get the parent index of a given position
@@ -163,7 +165,7 @@ contract MaxHeap is IMaxHeap, VersionedContract, UUPS, Ownable2StepUpgradeable,
     /// @dev This function adjusts the heap to maintain the max-heap property after updating the vote count
     function updateValue(uint256 itemId, uint256 newValue) public onlyAdmin {
         //ensure itemId exists in the heap
-        if (items[itemId].heapIndex >= size) revert INVALID_PIECE_ID();
+        if (items[itemId].heapIndex >= size) revert INVALID_ITEM_ID();
 
         uint256 position = items[itemId].heapIndex;
         uint256 oldValue = items[itemId].value;
@@ -175,8 +177,9 @@ contract MaxHeap is IMaxHeap, VersionedContract, UUPS, Ownable2StepUpgradeable,
         if (newValue > oldValue) {
             // Upwards heapify
             while (position != 0 && items[heap[position]].value > items[heap[parent(position)]].value) {
-                swap(position, parent(position));
-                position = parent(position);
+                uint256 parentOfPosition = parent(position);
+                swap(position, parentOfPosition);
+                position = parentOfPosition;
             }
         } else if (newValue < oldValue) maxHeapify(position); // Downwards heapify
     }

diff --git a/packages/revolution/src/interfaces/IAuctionHouse.sol b/packages/revolution/src/interfaces/IAuctionHouse.sol
@@ -76,6 +76,9 @@ interface IAuctionHouse is IAuctionHouseEvents {
     /// @dev Reverts if the creator rate is below the minimum required creator rate basis points.
     error CREATOR_RATE_TOO_LOW();
 
+    /// @dev Reverts if the reserve price is invalid.
+    error RESERVE_PRICE_INVALID();
+
     /// @dev Reverts if the new minimum creator rate is not greater than the previous minimum creator rate.
     error MIN_CREATOR_RATE_NOT_INCREASED();
 

diff --git a/packages/revolution/src/interfaces/IRevolutionPointsEmitter.sol b/packages/revolution/src/interfaces/IRevolutionPointsEmitter.sol
@@ -46,7 +46,7 @@ interface IRevolutionPointsEmitter is IRewardSplits {
     }
 
     struct BuyTokenPaymentShares {
-        uint256 buyersShare;
+        uint256 buyersGovernancePayment;
         uint256 creatorsDirectPayment;
         uint256 creatorsGovernancePayment;
     }

diff --git a/packages/revolution/test/auction/AuctionHouse.t.sol b/packages/revolution/test/auction/AuctionHouse.t.sol
@@ -22,7 +22,7 @@ import { ERC721CheckpointableUpgradeable } from "../../src/base/ERC721Checkpoint
 import { RevolutionBuilderTest } from "../RevolutionBuilder.t.sol";
 
 contract AuctionHouseTest is RevolutionBuilderTest {
-    function setUp() public override {
+    function setUp() public virtual override {
         super.setUp();
         super.setMockParams();
 

diff --git a/packages/revolution/test/auction/Basic.t.sol b/packages/revolution/test/auction/Basic.t.sol
@@ -184,10 +184,6 @@ contract AuctionHouseBasicTest is AuctionHouseTest {
     receive() external payable {}
 
     function testInitializationParameters() public {
-        emit log_string("testing");
-        emit log_address(weth);
-        emit log_address(auction.WETH());
-
         assertEq(auction.WETH(), address(weth), "WETH address should be set correctly");
         assertEq(auction.timeBuffer(), 15 minutes, "Time buffer should be set correctly");
         assertEq(auction.reservePrice(), 1 ether, "Reserve price should be set correctly");
@@ -379,6 +375,8 @@ contract AuctionHouseBasicTest is AuctionHouseTest {
         uint256 newReservePrice,
         uint8 newMinBidIncrementPercentage
     ) public {
+        newReservePrice = bound(newReservePrice, 1, 10_000_000 ether);
+
         auction.setTimeBuffer(newTimeBuffer);
         assertEq(auction.timeBuffer(), newTimeBuffer, "Time buffer should be updated correctly");
 

diff --git a/packages/revolution/test/auction/OutOfGas.t.sol b/packages/revolution/test/auction/OutOfGas.t.sol
@@ -9,9 +9,22 @@ import { MockWETH } from "../mock/MockWETH.sol";
 import { toDaysWadUnsafe } from "../../src/libs/SignedWadMath.sol";
 
 contract AuctionHouseOutOfGasTest is AuctionHouseTest {
+    function setUp() public override {
+        super.setUp();
+
+        // create 100k art pieces, reasonable upper bound for a single culture index
+        _createManyPieces(100_000);
+    }
+
+    function _createManyPieces(uint256 n) internal {
+        for (uint256 i = 0; i < n; i++) {
+            createDefaultArtPiece();
+        }
+    }
+
     // create an auction with a piece of art with given number of creators and finish it
     function _createAndFinishAuction() internal {
-        uint nCreators = cultureIndex.MAX_NUM_CREATORS();
+        uint256 nCreators = cultureIndex.MAX_NUM_CREATORS();
         address[] memory creatorAddresses = new address[](nCreators);
         uint256[] memory creatorBps = new uint256[](nCreators);
         uint256 totalBps = 0;
@@ -30,6 +43,17 @@ contract AuctionHouseOutOfGasTest is AuctionHouseTest {
             totalBps += creatorBps[i];
         }
 
+        //vote for tokenId
+        address voter = address(new InfiniteLoop());
+
+        vm.prank(address(revolutionPointsEmitter));
+
+        // mint points to voter
+        revolutionPoints.mint(voter, 1e10);
+
+        //vm roll
+        vm.roll(vm.getBlockNumber() + 1);
+
         // create the initial art piece
         uint256 tokenId = createArtPieceMultiCreator(
             createLongString(cultureIndex.MAX_NAME_LENGTH()),
@@ -44,20 +68,28 @@ contract AuctionHouseOutOfGasTest is AuctionHouseTest {
 
         vm.roll(vm.getBlockNumber() + 1); // roll block number to enable voting snapshot
 
+        // vote for the art piece
+        vm.prank(address(voter));
+        cultureIndex.vote(tokenId);
+
+        //assert totalVoteWeights for tokenId
+        uint256 totalVoteWeights = cultureIndex.totalVoteWeights(tokenId);
+        assertEq(totalVoteWeights, 1e10);
+
         vm.startPrank(auction.owner());
         auction.unpause();
         vm.stopPrank();
 
         uint256 bidAmount = auction.reservePrice();
         vm.deal(address(creators[nCreators - 1]), bidAmount + 1 ether);
         vm.startPrank(address(creators[nCreators - 1]));
-        auction.createBid{ value: bidAmount }(tokenId, address(creators[nCreators - 1]), address(0));
+        auction.createBid{ value: bidAmount }(0, address(creators[nCreators - 1]), address(0));
         vm.stopPrank();
 
         vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction
 
         // create another art piece so that it's possible to create next auction
-        createArtPieceMultiCreator(
+        uint256 tokenId2 = createArtPieceMultiCreator(
             createLongString(cultureIndex.MAX_NAME_LENGTH()),
             createLongString(cultureIndex.MAX_DESCRIPTION_LENGTH()),
             ICultureIndex.MediaType.ANIMATION,
@@ -68,6 +100,13 @@ contract AuctionHouseOutOfGasTest is AuctionHouseTest {
             creatorBps
         );
         vm.roll(vm.getBlockNumber() + 1); // roll block number to enable voting snapshot
+
+        //vote for tokenId2 to make sure it is auctioned next
+        vm.prank(address(voter));
+        cultureIndex.vote(tokenId2);
+
+        uint256 totalWeight2 = cultureIndex.totalVoteWeights(tokenId2);
+        assertEq(totalWeight2, 1e10);
     }
 
     // Helper function to create a string of a specified length
@@ -80,8 +119,11 @@ contract AuctionHouseOutOfGasTest is AuctionHouseTest {
     }
 
     //attempt to trigger an auction paused error with differing gas amounts
-    function test_OutOfGas_DOS(uint gasUsed) public {
+    /// forge-config: dev.fuzz.runs = 64
+    /// forge-config: ci.fuzz.runs = 64
+    function test_OutOfGasDOS(uint256 gasUsed) public {
         gasUsed = bound(gasUsed, 0, 31_000_000);
+
         vm.startPrank(cultureIndex.owner());
         cultureIndex._setQuorumVotesBPS(0);
         vm.stopPrank();