Tessera - Versus contest - gzeon's results

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/modules/GroupBuy.sol#L228

Vulnerability details

Impact

On a successful purchase, users can call claim on the GroupBuy contract to mint and refund. The refunded amount is userContributions - sum(minReservePrices*qty) - pendingBalances. In case of the actual purchase price < minReservePrices, the difference is not refunded.

Proof of Concept

In the testClaimSuccess test case, add the following line at the end:

        assertEq(address(groupBuy).balance, 0);

Only Alice, Bob and Eve contributed to the GroupBuy, so when they all claim the contract should be left with 0 ether. There are no other way to remove the excess ether in the contract.

Tools Used

Foundry

Recommended Mitigation Steps

After purchase, record the actual purchase price. Also need to think about rounding issues.

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L112-L116

Vulnerability details

Impact

The only check on a new proposal is that it is priced lower than the existing proposal. It does not constrain on the _collateral supplied (except it will revert in _verifyBalance if set to 0). Anyone can block normal proposal creation by creating a proposal with lower price but _collateral == 1. When a high total supply is used, the price of each Rae is negligible and enable an attacker to DOS the protocol.

This violated the prevent a user from holding a vault hostage and never letting the piece be reasonably bought requirement.

Proof of Concept

For any proposal, an attacker can deny it with _collateral = 1 and _price = price - 1

If he do not want the NFT to be sold, he can reject the proposal himself, reseting the contract state.

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L112-L116

        // Reverts if price per token is not lower than both the proposed and active listings
        if (
            _pricePerToken >= proposedListing.pricePerToken ||
            _pricePerToken >= activeListings[_vault].pricePerToken
        ) revert NotLower();

Add this test to OptimisticListingSeaport.t.sol:

    function testProposeRevertLowerTotalValue() public {
        uint256 _collateral = 100;
        uint256 _price = 100;
        // setup
        testPropose(_collateral, _price);
        lowerPrice = pricePerToken - 1;
        // execute
        vm.expectRevert();
        _propose(eve, vault, 1, lowerPrice, offer);
        // expect
        _assertListing(eve, 1, lowerPrice, block.timestamp);
        _assertTokenBalance(eve, token, tokenId, eveTokenBalance - 1);
    }

[FAIL. Reason: Call did not revert as expected]

Tools Used

Foundry

Recommended Mitigation Steps

Require the total value of the new collateral be greater than the previous. This however still allow a Rae holder with sufficiently large holding to block proposal by creating a new proposal and immediately reject it himself.

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/modules/GroupBuy.sol#L186-L188

Vulnerability details

Impact

When _purchaseProof.length == 0, GroupBuy.purchase compare the tokenId with the merkleRoot. This allow any tokenId that match the merkleRoot to be purchased, even if they are not included in the allow list during setup.

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/modules/GroupBuy.sol#L186-L188

        if (_purchaseProof.length == 0) {
            // Hashes tokenId to verify merkle root if proof is empty
            if (bytes32(_tokenId) != merkleRoot) revert InvalidProof();

Proof of Concept

Add the following to GroupBuy.t.sol, it would still revert (since no such nft existed) but not as expected.

    // modified from testPurchaseRevertInvalidProof
    function testPurchaseRevertInvalidTokenIdZeroLength() public {
        // setup
        testContributeSuccess();
        // exploit
        uint256 invalidPunkId = uint256(nftMerkleRoot);
        bytes32[] memory invalidPurchaseProof = new bytes32[](0);
        // expect
        vm.expectRevert(INVALID_PROOF_ERROR);
        // execute
        _purchase(
            address(this),
            currentId,
            address(punksBuyer),
            address(punks),
            invalidPunkId,
            minValue,
            purchaseOrder,
            invalidPurchaseProof
        );
    }

Tools Used

Foundry

Recommended Mitigation Steps

Hash the tokenId even if there is only when length is 1

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L145

Vulnerability details

Impact

In OptimisticListingSeaport.rejectProposal, it revert if proposedListing.collateral < _amount. An attacker can therefore monitor the mempool, reducing the proposedListing.collateral to _amount - 1 by frontruning the rejectProposal call and delay the rejection. The attacker may even be able to deny the rejection when the deadline passes.

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L145

        if (proposedListing.collateral < _amount) revert InsufficientCollateral();

https://github.com/code-423n4/2022-12-tessera/blob/f37a11407da2af844bbfe868e1422e3665a5f8e4/src/seaport/modules/OptimisticListingSeaport.sol#L153

        proposedListing.collateral -= _amount;

Proof of Concept

1. Attacker propose at 10000 collateral at a very low price
2. Bob try to reject it by purchasing the 10000 collateral
3. Attacker see Bob's tx in the mempool, frontrun it to reject 1 unit
4. The proposedListing.collateral is now 9999
5. Bob's call reverted
6. This keep happening until PROPOSAL_PERIOD pass or Bob gave up because of gas paid on failing tx
7. Attacker buy the NFT at a very low price

Recommended Mitigation Steps

When proposedListing.collateral < _amount, set _amount to proposedListing.collateral and refund the excess.

Judge has assessed an item in Issue #22 as M risk. The relevant finding follows:

[NC-2] Return values of approve() not checked

Judge has assessed an item in Issue #22 as M risk. The relevant finding follows:

minBidPrices is rounded down

QA Report

Disclaimer: report extended from 4naly3er tool

Low Issues

Non Critical Issues

<a name="L-1"></a>[L-1] Unsafe ERC20 operation(s)

Instances (1):

File: src/seaport/targets/SeaportLister.sol

40:                         IERC20(token).approve(conduit, type(uint256).max);

Link to code

<a name="L-2"></a>[L-2] Contribution to GroupBuy should check against minReservePrices

If the bid is below minReservePrices it will be filtered

Instances (1):

        if (msg.value < _quantity * minBidPrices[_poolId] || _quantity == 0)

<a name="L-3"></a>[L-3] Onchain merkle tree generation limited the size of tokenId

Generate merkle tree onchain is expensive espically when you want to include a large set of value. For example, it is not possible to include all Punk in the tokenId becuase the merkle root is generated onchain. Consider generate it offchain and publish the root.

        bytes32 merkleRoot = (length == 1) ? bytes32(_tokenIds[0]) : _generateRoot(_tokenIds);

Link to code

<a name="NC-1"></a>[NC-1] Missing checks for address(0) when assigning values to address state variables

Instances (9):

File: src/punks/protoforms/PunksMarketBuyer.sol

32:         registry = _registry;

33:         wrapper = _wrapper;

34:         listing = _listing;

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

71:         registry = _registry;

72:         seaport = _seaport;

73:         zone = _zone;

75:         supply = _supply;

76:         seaportLister = _seaportLister;

Link to code

File: src/seaport/targets/SeaportLister.sol

20:         conduit = _conduit;

Link to code

<a name="NC-2"></a>[NC-2] Return values of approve() not checked

Not all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

Instances (1):

File: src/seaport/targets/SeaportLister.sol

40:                         IERC20(token).approve(conduit, type(uint256).max);

Link to code

<a name="NC-3"></a>[NC-3] Functions not used internally could be marked external

Instances (11):

File: src/lib/MinPriorityQueue.sol

27:     function initialize(Queue storage self) public {

36:     function getNumBids(Queue storage self) public view returns (uint256) {

41:     function getMin(Queue storage self) public view returns (Bid storage) {

90:     function delMin(Queue storage self) public returns (Bid memory) {

Link to code

File: src/modules/GroupBuy.sol

346:     function getBidInQueue(uint256 _poolId, uint256 _bidId)

371:     function getNextBidId(uint256 _poolId) public view returns (uint256) {

377:     function getNumBids(uint256 _poolId) public view returns (uint256) {

384:     function getBidQuantity(uint256 _poolId, uint256 _bidId) public view returns (uint256) {

402:     function printQueue(uint256 _poolId) public view {

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

218:     function list(address _vault, bytes32[] calldata _listProof) public {

344:     function getPermissions()

Link to code

<a name="NC-4"></a>[NC-4] minBidPrices is rounded down

minBidPrices is rounded down and can be 0, which will cause precision issue

        minBidPrices[currentId] = _initialPrice / _totalSupply;

Link to code

<a name="NC-5"></a>[NC-5] Member of _tokenIds might not be unique

        uint256[] calldata _tokenIds,

Link to code

<a name="NC-6"></a>[NC-6] Do not use assert() in production

It will consume all gas and does not provide a revert string, use require() instead

        assert(ConsiderationInterface(_consideration).validate(_orders));

Link to code

        assert(ConsiderationInterface(_consideration).cancel(_orders));

Link to code

<a name="NC-7"></a>[NC-7] Hardcoded fees

Would require migration if either decided to change fee structure in the future.

        uint256 openseaFees = _listingPrice / 40;
        uint256 tesseraFees = _listingPrice / 20;

Link to code

<a name="NC-8"></a>[NC-8] Wrong comments

Here should be + 2 consideration for the fees

            // 1 Consideration for the listing itself + 1 consideration for the fees

Link to code

Gas Optimization Report

Disclaimer: report extended from 4naly3er tool

Gas Optimizations

<a name="GAS-1"></a>[GAS-1] Use assembly to check for address(0)

Saves 6 gas per instance

Instances (2):

File: src/seaport/modules/OptimisticListingSeaport.sol

106:             proposedListings[_vault].proposer == address(0) &&

107:             activeListings[_vault].proposer == address(0)

Link to code

<a name="GAS-2"></a>[GAS-2] Cache array length outside of loop

If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).

Instances (2):

File: src/lib/MinPriorityQueue.sol

98:         for (uint256 i = 0; i < curUserBids.length; i++) {

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

390:             for (uint256 i = 0; i < _offer.length; ++i) {

Link to code

<a name="GAS-3"></a>[GAS-3] State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

Saves 100 gas per instance

Instances (5):

File: src/modules/GroupBuy.sol

92:         contribute(currentId, _quantity, _raePrice);

Link to code

File: src/punks/protoforms/PunksMarketBuyer.sol

37:         proxy = IWrappedPunk(wrapper).proxyInfo(address(this));

64:         IERC721(wrapper).safeTransferFrom(address(this), vault, tokenId);

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

362:             seaportLister,

Link to code

File: src/seaport/targets/SeaportLister.sol

38:                         IERC1155(token).setApprovalForAll(conduit, true);

Link to code

<a name="GAS-4"></a>[GAS-4] Use calldata instead of memory for function arguments that do not get mutated

Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.

Instances (5):

File: src/modules/GroupBuy.sol

166:         bytes memory _purchaseOrder,

167:         bytes32[] memory _purchaseProof

Link to code

File: src/punks/protoforms/PunksMarketBuyer.sol

46:     function execute(bytes memory _order) external payable returns (address vault) {

Link to code

File: src/seaport/targets/SeaportLister.sol

26:     function validateListing(address _consideration, Order[] memory _orders) external {

51:     function cancelListing(address _consideration, OrderComponents[] memory _orders) external {

Link to code

<a name="GAS-5"></a>[GAS-5] Use Custom Errors

Source Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.

Instances (2):

File: src/lib/MinPriorityQueue.sol

42:         require(!isEmpty(self), "nothing to return");

91:         require(!isEmpty(self), "nothing to delete");

Link to code

<a name="GAS-6"></a>[GAS-6] Don't initialize variables with default value

Instances (2):

File: src/lib/MinPriorityQueue.sol

98:         for (uint256 i = 0; i < curUserBids.length; i++) {

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

390:             for (uint256 i = 0; i < _offer.length; ++i) {

Link to code

<a name="GAS-7"></a>[GAS-7] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (4):

File: src/lib/MinPriorityQueue.sol

60:                 j++;

77:         insert(self, Bid(self.nextBidId++, owner, price, quantity));

98:         for (uint256 i = 0; i < curUserBids.length; i++) {

Link to code

File: src/modules/GroupBuy.sol

74:         poolInfo[++currentId] = PoolInfo(

Link to code

<a name="GAS-8"></a>[GAS-8] Use shift Right/Left instead of division/multiplication if possible

Instances (5):

File: src/lib/MinPriorityQueue.sol

49:         while (k > 1 && isGreater(self, k / 2, k)) {

50:             exchange(self, k, k / 2);

51:             k = k / 2;

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

395:         uint256 openseaFees = _listingPrice / 40;

396:         uint256 tesseraFees = _listingPrice / 20;

Link to code

<a name="GAS-9"></a>[GAS-9] Use != 0 instead of > 0 for unsigned integer comparison

Instances (5):

File: src/modules/GroupBuy.sol

124:         if (fillAtAnyPriceQuantity > 0) {

139:         if (filledQuantity > 0) minReservePrices[_poolId] = getMinPrice(_poolId);

268:         if (pendingBalances[msg.sender] > 0) withdrawBalance();

297:         while (quantity > 0) {

Link to code

File: src/seaport/modules/OptimisticListingSeaport.sol

469:         if (isValidated && !isCancelled && totalFilled > 0 && totalFilled == totalSize) {

Link to code

<a name="GAS-10"></a>[GAS-10] Use unchecked when it is safe

For example, those dealing with eth value will basically never overflow:

        userContributions[_poolId][msg.sender] += msg.value;
        totalContributions[_poolId] += msg.value;

Link to code

            filledQuantities[_poolId] += fillAtAnyPriceQuantity;

Link to code

Also those with explicit check before

                lowestBid.quantity -= quantity;

Link to code

<a name="GAS-11"></a>[GAS-11] Refund contribution and pending balance in the same call

        // Transfers remaining contribution balance back to caller
        payable(msg.sender).call{value: contribution}("");

        // Withdraws pending balance of caller if available
        if (pendingBalances[msg.sender] > 0) withdrawBalance();

Link to code

<a name="GAS-12"></a>[GAS-12] Generate merkle tree offchain

Generate merkle tree onchain is expensive espically when you want to include a large set of value. Consider generating it offchain a publish the root when creating a new pool.

        bytes32 merkleRoot = (length == 1) ? bytes32(_tokenIds[0]) : _generateRoot(_tokenIds);

Link to code

<a name="GAS-13"></a>[GAS-13] Pack Bid Struct

The Bid struct can be packed tighter

struct Bid {
    uint256 bidId;
    address owner;
    uint256 price;
    uint256 quantity;
}

Link to code to

struct Bid {
    uint96 bidId;
    address owner;
    uint256 price;
    uint256 quantity;
}

<a name="GAS-14"></a>[GAS-14] Pack Queue Struct

    struct Queue {
        ///@notice incrementing bid id
        uint256 nextBidId;
        ///@notice array backing priority queue
        uint256[] bidIdList;
        ///@notice total number of bids in queue
        uint256 numBids;
        //@notice map bid ids to bids
        mapping(uint256 => Bid) bidIdToBidMap;
        ///@notice map addreses to bids they own
        mapping(address => uint256[]) ownerToBidIds;
    }

Link to code to

    struct Queue {
        ///@notice incrementing bid id
        uint96 nextBidId;
        ///@notice total number of bids in queue
        uint160 numBids;
        ///@notice array backing priority queue
        uint256[] bidIdList;
        //@notice map bid ids to bids
        mapping(uint256 => Bid) bidIdToBidMap;
        ///@notice map addreses to bids they own
        mapping(address => uint256[]) ownerToBidIds;
    }