Tessera - Versus contest - Trust's results

Lines of code

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

Vulnerability details

Description

claim() function is used in GroupBuy to mint Raes proportional to user's contribution to the purchased NFT. withdrawBalance() is used to get back funds which are not part of the contribution.

They both contain an unsafe call with ETH. For example:

function withdrawBalance() public {
    // Reverts if caller balance is insufficient
    uint256 balance = pendingBalances[msg.sender];
    if (balance == 0) revert InsufficientBalance();
    // Resets pending balance amount
    delete pendingBalances[msg.sender];
    // Transfers pending ether balance to caller
    payable(msg.sender).call{value: balance}("");
}

If msg.sender is a contract that does not implement a fallback function, the pending balance would be permanently frozen in GroupBuy contract. There are no warnings or validations that user is an EOA, so the call return value must be checked. It makes sense for contracts to be built around GroupBuy so it is a necessary validation.

Impact

When user of GroupBuy is a contract, refunds will be permanently frozen.

Tools Used

Manual audit

Recommended Mitigation Steps

2 options:

1. Do not allow contracts to interact with GroupBuy (by verifying msg.sender == tx.origin)
2. Allow contracts to use GroupBuy, but they must supply a pull-refunds EOA at the contribute stage.

Lines of code

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

Vulnerability details

Description

In OptimisticListingSeaport.sol, users call propose() to set a new proposal. The pendingBalance is updated immediately for the calling proposer:

// Sets collateral amount to pending balances for withdrawal
pendingBalances[_vault][proposedListing.proposer] += proposedListing.collateral;

This is the amount transferred from user to contract:

// Transfers new collateral amount from caller to this contract
IERC1155(token).safeTransferFrom(msg.sender, address(this), id, _collateral, "");

However, any value in pendingBalances is instantly claimable. Using withdrawCollateral, user will simply take back their collateral. There is no additional bookkeeping, like done in rejectActive and rejectProposal flow, which cancel the proposal when there is no collateral defending it.

function withdrawCollateral(address _vault, address _to) external {
    // Reverts if vault is not registered
    (address token, uint256 id) = _verifyVault(_vault);
    // Reverts if token balance is insufficient
    uint256 balance = pendingBalances[_vault][_to];
    if (balance == 0) revert NotEnoughTokens();
    // Resets collateral balance amount
		// ----- simply transfer out balance here -----
    pendingBalances[_vault][_to] = 0;
    // Transfers collateral amount to receiver
    IERC1155(token).safeTransferFrom(address(this), _to, id, balance, "");
    // Emits event for withdrawing collateral balance
    emit WithdrawCollateral(_vault, _to, balance);
}

Reject proposal logic:

// Decrements collateral amount
proposedListing.collateral -= _amount;
// Checks if proposed listing has been rejected
if (proposedListing.collateral == 0) {
    // Resets proposed listing to default
    _setListing(proposedListing, address(this), 0, type(uint256).max, 0);
}

Impact

User can send a proposal and instantly take back their collateral, keeping the proposal active without risking any Raes amount.

Tools Used

Manual audit

Recommended Mitigation Steps

It is best not to add pendingBalance for proposer in propose() function. Use the rejectProposal, rejectActive and cash functions to update pendingBalance when we know how much of user's collateral should be given back to them.

Lines of code

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

Vulnerability details

Description

OptimisticListingSeaport.sol is easily DOSable due to the following conditions:

1. A new proposal overrides the existing proposal
2. The deposited collateral can be any non-zero amount
3. Only requirement is that _pricePerToken is lower than before. In 10**18 sized numbers, this gives a nearly unlimited amount of space for decrement

Therefore, an attacker can continually reset an honest proposal and never let it reach maturity and be activated.

Impact

Proposal can be infinitely DOSed with no additional conditions

Proof of Concept

1. Honest proposal is sent to listing contract
2. User waits a little less than PROPOSAL_PERIOD time
3. User sends a proposal with 1 collateral token and _pricePerToken = honest pricePerToken - 1. We reset the previous proposal maturity time.
4. Repeat step 2

Tools Used

Manual audit

Recommended Mitigation Steps

The standard practice in this type of scenario is allow multiple proposals to run simultaneously. Once enough time has passed the proposal will be promoted to activeProposal and can be executed.

Lines of code

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

Vulnerability details

Description

OptimisticListingSeaport exposes propose() method to create new proposal, and rejectProposal to remove a listing in proposal stage.

In propose, proposer commits a certain amount of collateral:

// Sets collateral amount to pending balances for withdrawal
pendingBalances[_vault][proposedListing.proposer] += proposedListing.collateral;
// Transfers new collateral amount from caller to this contract
IERC1155(token).safeTransferFrom(msg.sender, address(this), id, _collateral, "");

In rejectProposal, user pays proposer msg.value which is the ETH equivalent of amount stored in collateral. In return they get the Raes collateral.

// Transfers tokens to caller
IERC1155(token).safeTransferFrom(address(this), msg.sender, id, _amount, "");
// Sends ether to proposer
_sendEthOrWeth(proposer, msg.value);
// Emits event for rejecting a proposed listing
emit RejectProposal(_vault, msg.sender, _amount, msg.value, proposedListing.order);

The issue is that these two actions are zero sum and can be executed without time constraints. Therefore, they can be used by a single party to infinitely DOS any proposal and not let any proposal become active. Effectively it is an NFT freeze of funds.

Impact

Any user which holds Raes tokens can infinitely freeze NFT in OptimisticListingSeaport

Proof of Concept

1. An innocent user creates a proposal to sell the NFT
2. An attacker calls propose(). They don't need much Raes as collateral is not lower-bounded.
3. Attacker waits as long as they wait. Eventually they can call rejectProposal(). They pass some ETH amount, which is transferred to the proposer, which is themselves. They receive the collateral deposited earlier. They are net zero.
4. This loop can be repeated as many times as attacker wishes. Each iteration of the loop freezes the NFT for PROPOSAL_PERIOD time, the time for an innocent proposal to mature. A moment before maturity they will call propose() again.

Tools Used

Manual audit

Recommended Mitigation Steps

Add some penalty for rejected proposals which will make the attack not worthwhile. Additionally, make the collateral be a significant amount to limit the number of potential attackers.

Lines of code

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

Vulnerability details

Description

_constructOrder is called in propose(), OptimisticListingSeaport.sol. It fills the order params stored in proposedListings[_vault].

{
    orderParams.offerer = _vault;
    orderParams.startTime = block.timestamp;
    // order doesn't expire in human time scales and needs explicit cancellations
    orderParams.endTime = type(uint256).max;
    orderParams.zone = zone;
    // 0: no partial fills, anyone can execute
    orderParams.orderType = OrderType.FULL_OPEN;
    orderParams.conduitKey = conduitKey;
    // 1 Consideration for the listing itself + 1 consideration for the fees
    orderParams.totalOriginalConsiderationItems = 3;
}

Importantly, it updates the order hash associated with the vault: vaultOrderHash[_vault] = _getOrderHash(orderParams, counter);

There is only one other use of vaultOrderHash, in _verifySale().

function _verifySale(address _vault) internal view returns (bool status) {
    (bool isValidated, bool isCancelled, uint256 totalFilled, uint256 totalSize) = ISeaport(
        seaport
    ).getOrderStatus(vaultOrderHash[_vault]);
    if (isValidated && !isCancelled && totalFilled > 0 && totalFilled == totalSize) {
        status = true;
    }
}

This function gets order information from the order hash, and makes sure the order is completely fulfilled.

After NFT sell has completed, cash() is used to distribute income ETH:

function cash(address _vault, bytes32[] calldata _burnProof) external {
    // Reverts if vault is not registered
    (address token, uint256 id) = _verifyVault(_vault);
    // Reverts if active listing has not been settled
    Listing storage activeListing = activeListings[_vault];
    // Reverts if listing has not been sold
	  // -------------- _verifySale MUST BE TRUE ---------
    if (!_verifySale(_vault)) {
        revert NotSold();
    } else if (activeListing.collateral != 0) {
        uint256 collateral = activeListing.collateral;
        activeListing.collateral = 0;
        // Sets collateral amount to pending balances for withdrawal
        pendingBalances[_vault][activeListing.proposer] = collateral;
    }

As long as sale is not complete, cash() can't be called as highlighted. The issue is that vaultOrderHash[_vault] is not protected during the lifetime of an active proposal. If another proposal is proposed and then the sell using active proposal takes place, cash() will keep reverting. Funds are stuck in listing contract.

We can try to be clever and call propose() again with the same parameters to create an identical orderID, which will make vaultOrderHash[_vault] fine again and allow cash() to go through. But order params contain block.timestamp which will certainly be different which will make the hash different.

Impact

Funds are permanently stuck in OptimisticListingSeaport.sol contract if active proposal is executed after new proposal is pending.

Proof of Concept

1. User A calls propose(), setting proposedListing. vaultOrderHash=X
2. PROPOSAL_PERIOD passes , list is called promoting the listing to activeListing.
3. Another user, malicious or innocent, proposes another proposal. vaultOrderHash=Y
4. Sell goes down due to OpenSea validation confirmed on activeListing.
5. _verifySale will never return true because we can never got vaultOrderHash to be X
6. cash() is bricked. Money is stuck in contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Keep the order hash in the Listing structure rather than a single one per vault.

Lines of code

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

Vulnerability details

Description

In OptimisticListingOpensea, there are several functions which update pendingBalances of a proposer:

1. list()
2. cash()
3. propose()

Unfortunately, in list() and cash() the = operator is used instead of += when writing the new pendingBalances. For example:

function cash(address _vault, bytes32[] calldata _burnProof) external {
    // Reverts if vault is not registered
    (address token, uint256 id) = _verifyVault(_vault);
    // Reverts if active listing has not been settled
    Listing storage activeListing = activeListings[_vault];
    // Reverts if listing has not been sold
    if (!_verifySale(_vault)) {
        revert NotSold();
    } else if (activeListing.collateral != 0) {
        uint256 collateral = activeListing.collateral;
        activeListing.collateral = 0;
        // Sets collateral amount to pending balances for withdrawal
        pendingBalances[_vault][activeListing.proposer] = collateral;
    }
	...

pendingBalances is not guaranteed to be zero. There could be funds from previous proposals which are not yet collected. Propose updates pendingBalance correctly:

// Sets collateral amount to pending balances for withdrawal
pendingBalances[_vault][proposedListing.proposer] += proposedListing.collateral;

So, when propose is followed by another propose(), the pendingBalance is updated correctly, but in cash and list we don't account for pre-existing balance. This issue would manifest even after the fix suggested in the issue "User can send a proposal and instantly take back their collateral" because reject functions would increment the pendingBalance and then it would be overriden.

Impact

User loses collateral converted to pendingBalance when cash() or list() is called

Proof of Concept

1. User calls propose() and gets pendingBalance = x
2. User calls propose() with an improved proposal and gets pendingBalance = 1.5x
3. proposal is successfull and the listing purchased the NFT
4. cash() is called to convert the Raes to ETH amount from the sell. pendingBalance is overridden by the current "collateral" value. pendingBalance = 0.5x
5. User loses x collateral value which is stuck in the contract

Tools Used

Manual audit

Recommended Mitigation Steps

Change the = operator to += in list() and cash().

Lines of code

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

Vulnerability details

Description

purchase() in GroupBuy.sol executes the purchase call for the group. After safety checks, the NFT is bought with _market's execute() function. Supposedly it deploys a vault which owns the NFT. The code makes sure the vault is the new owner of the NFT and exits.

// Executes purchase order transaction through market buyer contract and deploys new vault
address vault = IMarketBuyer(_market).execute{value: _price}(_purchaseOrder);
// Checks if NFT contract supports ERC165 and interface ID of ERC721 tokens
if (ERC165Checker.supportsInterface(_nftContract, _INTERFACE_ID_ERC721)) {
    // Verifes vault is owner of ERC-721 token
    if (IERC721(_nftContract).ownerOf(_tokenId) != vault) revert UnsuccessfulPurchase();
} else {
    // Verifies vault is owner of CryptoPunk token
    if (ICryptoPunk(_nftContract).punkIndexToAddress(_tokenId) != vault)
        revert UnsuccessfulPurchase();
}

// Stores mapping value of poolId to newly deployed vault
poolToVault[_poolId] = vault;
// Sets pool state to successful
poolInfo[_poolId].success = true;
// Emits event for purchasing NFT at given price
emit Purchase(_poolId, vault, _nftContract, _tokenId, _price);

The issue is that _market user-supplied variable is not validated at all. Attacker can pass their malicious contract, which uses the passed funds to buy the NFT and store it in attacker's wallet. It will return the NFT-holding wallet so the checks will pass. As a result, attacker has the NFT while they could have contributed nothing to the GroupBuy. Attacker can also just steal the supplied ETH and return the current address which holds the NFT.

Impact

Attacker can steal the amount collected so far in the GroupBuy for NFT purchase.

Proof of Concept

1. Group assembles and raises funds to buy NFT X
2. Attacker calls purchase() and supplies their malicious contract in _market, as described.
3. Attacker receives raised funds totalling minReservePrices[_poolId] * filledQuantities[_poolId], as checked in line 182.

Tools Used

Manual audit

Recommended Mitigation Steps

_market should be whitelisted, or supplied in createPool stage and able to be scrutinized.

Lines of code

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

Vulnerability details

Description

purchase() in GroupBuy faciilitates the purchasing of an NFT after enough contributions were gathered. Another report titled "Attacker can steal the amount collected so far in the GroupBuy for NFT purchase" describes a high impact bug in purchase. It is advised to read that first for context.

Additionally, purchase() is vulnerable to a re-entrancy exploit which can be chained or not chained to the _market issue to steal the entire ETH stored in GroupBuy, rather than being capped to minReservePrices[_poolId] * filledQuantities[_poolId].

Attacker may take control of execution using this call:

// Executes purchase order transaction through market buyer contract and deploys new vault
address vault = IMarketBuyer(_market).execute{value: _price}(_purchaseOrder);

It could occur either by exploiting the unvalidated _market vulnerability , or by abusing an existing market that uses a user address in _purchaseOrder.

There is no re-entrancy protection in purchase() call:

function purchase(
    uint256 _poolId,
    address _market,
    address _nftContract,
    uint256 _tokenId,
    uint256 _price,
    bytes memory _purchaseOrder,
    bytes32[] memory _purchaseProof
) external {

_verifyUnsuccessfulState() needs to not revert for purchase call. It checks the pool.success flag: if (pool.success || block.timestamp > pool.terminationPeriod) revert InvalidState();

However, success is only set as the last thing in purchase():

    // Stores mapping value of poolId to newly deployed vault
    poolToVault[_poolId] = vault;
    // Sets pool state to successful
    poolInfo[_poolId].success = true;
    // Emits event for purchasing NFT at given price
    emit Purchase(_poolId, vault, _nftContract, _tokenId, _price);
}

Therefore, attacker can re-enter purchase() function multiple times, each time extracting the maximum allowed price. If attacker uses the controlled _market exploit, the function will return the current NFT owner, so when all the functions unwind they will keep setting success to true and exit nicely.

Impact

GroupBuy can be drained of all ETH.

Proof of Concept

1. GroupBuy holds 1500 ETH, from various bids
2. maximum allowed price (minReservePrices[_poolId] * filledQuantities[_poolId]) is 50 * 20 = 1000 ETH
3. purchase(1000 ETH) is called
   1. GroupBuy sends attacker 1000 ETH and calls execute()
      1. execute() calls purchase(500ETH)
         1. GroupBuy sends attacker 500 ETH and calls execute()
            1. execute returns NFT owner address
         2. GroupBuy sees returned address is NFT owner. Marks success and returns
      2. execute returns NFT owner address
   2. GroupBuy sees returned address is NFT owner. Marks success and returns
4. Attacker is left with 1500 ETH. Previous exploit alone can only net 1000ETH. Additionally, this exploit can be chained to any trusted MarketBuyer which passes control to user for purchasing and storing in vault, and then returns a valid vault.

Tools Used

Manual audit

Recommended Mitigation Steps

Add a re-entrancy guard to purchase() function. Also, change success variable before performing external contract calls.

Lines of code

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

Vulnerability details

Description

purchase() in GroupBuy.sol executes the purchase call for the group. There are two possibilities for which tokenIDs can be bought in GroupBuy:

1. If the group is for a specific NFT, the tokenID is the value in pool.merkleRoot variable.
2. If the group supports multiple NFTs, the tokenID must be a leaf in pool.merkleRoot

The merkleRoot value is defined in createPool: bytes32 merkleRoot = (length == 1) ? bytes32(_tokenIds[0]) : _generateRoot(_tokenIds);

purchase() receives a _purchaseProof bytes32[] variable. It verifies the tokenID being bought is pre-accepted in this code snippet:

// Checks merkle proof based on size of array
if (_purchaseProof.length == 0) {
    // Hashes tokenId to verify merkle root if proof is empty
    if (bytes32(_tokenId) != merkleRoot) revert InvalidProof();
} else {
    // Verifies merkle proof based on position of leaf node in tree
    bytes32 leaf = keccak256(abi.encode(_tokenId));
    if (!MerkleProof.verify(_purchaseProof, merkleRoot, leaf)) revert InvalidProof();
}

The issue is that merkleRoot is treated as a merkleRoot or as an inline tokenID based on the existence of _purchaseProof which is supplied by the user. Therefore, attacker may pass a _purchaseProof when merkleRoot is intended to be a tokenId to treat the tokenId as a merkle root. More severely, when merkleRoot is intended as a true merkleRoot, attacker can pass a zero proof and copy the merkleRoot value in tokenId. It will be accepted in this check: if (bytes32(_tokenId) != merkleRoot)

The impact is that attacker may use the group's funds to buy a non intended tokenId. It will be some random uint256 number attacker doesn't control. The best way to exploit the error is to use NFT collections with user-chosen tokenIds. The group would raise money to buy a high value token, while user will mint a worthless one and use the group funds on it.

Impact

Attacker can make group pay for tokenID that is not intended.

Proof of Concept

1. NFT Collection C offers user-picked tokenID mints
2. Victim calls createPool and choses tokens A,B,C. A merkleTree is stored which allows A,B,C purchases
3. Additional victims contribute to the pool
4. Once a lot of funds are deposited, attacker mints NFT with tokenID = merkleRoot(A,B,C). They put it up for sale in the relevant marketplace.
5. Attacker passes zero purchaseProof and calls purchase. purchase() treats merkleRoot field as tokenId and will buy the worthless NFT.
6. Attacker profits the collected funds in the group.

Tools Used

Manual audit

Recommended Mitigation Steps

Add an additional variable in PoolInfo struct, to note if there are multiple tokenIds or single one.

Lines of code

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

Vulnerability details

Description

processBidsInQueue in GroupBuy handles integrating a new bid into the existing structure. If bumps out lower-paying bids until the new bid is completely fulfilled or the remaining bids are too high.

When the lowest paying bid has higher quantity than the remaining bid quantity to satisfy, we only bump out the necessary amount and leave it at the head of the priority queue:

if (lowestBidQuantity > quantity) {
    // Decrements given quantity amount from lowest bid quantity
    lowestBid.quantity -= quantity;
    // Calculates partial contribution of bid by quantity amount and price
    uint256 contribution = quantity * lowestBid.price;
    // Decrements partial contribution amount of lowest bid from total and user contributions
    totalContributions[_poolId] -= contribution;
    userContributions[_poolId][lowestBid.owner] -= contribution;
    // Increments pending balance of lowest bid owner
    pendingBalances[lowestBid.owner] += contribution;
    // Inserts new bid with given quantity amount into proper position of queue
    bidPriorityQueues[_poolId].insert(msg.sender, _price, quantity);
    // Resets quantity amount to exit while loop
    quantity = 0;

The line that changes the bid is lowestBid.quantity -= quantity;. The issue is that this behavior is unfair and contrary to the docs. It is stated that "If two users place bids at the same price but with different quantities, the queue will pull from the bid with a higher quantity first."

After removing "quantity" amount from lowestBid, it may now be a smaller quantity than others with the same price. Therefore, it should not be the minimum node at this point. The heap needs to re-organize and place the highest remaining quantity next in the queue. This would retain fairness and not completely remove one bid while another has much higher quantity.

Impact

Bidding data structure is corrupted, leading to unfair removal from queue.

Tools Used

Manual audit

Recommended Mitigation Steps

Use the sink() queue function to update the data structure when not completely removing a node from the tree.

Lines of code

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

Vulnerability details

Description

sendEthOrWeth() is used in several locations in OptimisticListingSeaport:

1. rejectProposal - sent to proposer
2. rejectActive - sent to proposer
3. cash - sent to msg.sender

This is the implementation of sendEthOrWeth:

function _attemptETHTransfer(address _to, uint256 _value) internal returns (bool success) {
    // Here increase the gas limit a reasonable amount above the default, and try
    // to send ETH to the recipient.
    // NOTE: This might allow the recipient to attempt a limited reentrancy attack.
    (success, ) = _to.call{value: _value, gas: 30000}("");
}
/// @notice Sends eth or weth to an address
/// @param _to Address to send to
/// @param _value Amount to send
function _sendEthOrWeth(address _to, uint256 _value) internal {
    if (!_attemptETHTransfer(_to, _value)) {
        WETH(WETH_ADDRESS).deposit{value: _value}();
        WETH(WETH_ADDRESS).transfer(_to, _value);
    }
}

The issue is that the receive could be a contract that does not have a fallback function. In this scenario, _attemptETHTransfer will fail and WETH would be transferred to the contract. It is likely that it bricks those funds for the contract as there is no reason it would support interaction with WETH tokens.

It can be reasonably assumed that developers will develop contracts which will interact with OptimisticListingSeaport using proposals. They are not warned and are likely to suffer losses.

Impact

Loss of ETH for proposer when it is a contract that doesn't have fallback function.

Tools Used

Manual audit

Recommended Mitigation Steps

Either enforce that proposer is an EOA or take in a recipient address for ETH transfers.

Lines of code

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

Vulnerability details

Description

In GroupBuy module, users can call contribute to get a piece of the NFT pie. There are two stages in transforming the msg.value to holdings in the NFT. 

1. filling at any price(supply is not yet saturated)

uint256 fillAtAnyPriceQuantity = remainingSupply < _quantity ? remainingSupply : _quantity;
// Checks if quantity amount being filled is greater than 0
if (fillAtAnyPriceQuantity > 0) {
    // Inserts bid into end of queue
    bidPriorityQueues[_poolId].insert(msg.sender, _price, fillAtAnyPriceQuantity);
    // Increments total amount of filled quantities
    filledQuantities[_poolId] += fillAtAnyPriceQuantity;
}

2. Trim out lower price offers to make room for current higher offer.

// Calculates unfilled quantity amount based on desired quantity and actual filled quantity amount
uint256 unfilledQuantity = _quantity - fillAtAnyPriceQuantity;
// Processes bids in queue to recalculate unfilled quantity amount
unfilledQuantity = processBidsInQueue(_poolId, unfilledQuantity, _price);

The while loop in processBidsInQueue will keep removing existing bids with lower price and create new queue entries for currently processed bid. When it reached a bid with a higher price than msg.sender's price, it will break:

while (quantity > 0) {
    // Retrieves lowest bid in queue
    Bid storage lowestBid = bidPriorityQueues[_poolId].getMin();
    // Breaks out of while loop if given price is less than than lowest bid price
    if (_price < lowestBid.price) {
        break;
    }

The issue is that when _price == lowestBid.price, we don't break and current bid will kick out older bid, as can be seen here:

// Decrements given quantity amount from lowest bid quantity
lowestBid.quantity -= quantity;
// Calculates partial contribution of bid by quantity amount and price
uint256 contribution = quantity * lowestBid.price;
// Decrements partial contribution amount of lowest bid from total and user contributions
totalContributions[_poolId] -= contribution;
userContributions[_poolId][lowestBid.owner] -= contribution;
// Increments pending balance of lowest bid owner
pendingBalances[lowestBid.owner] += contribution;
// Inserts new bid with given quantity amount into proper position of queue
bidPriorityQueues[_poolId].insert(msg.sender, _price, quantity);

The described behavior goes against what the docs describe will happen when two equal priced bids collide.

Impact

Earlier bidders get cut out of future NFT holdings by bidders specifying the same price.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the < to <= in the if condition:

if (_price <= lowestBid.price) {
    break;
}

Lines of code

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

Vulnerability details

Description

createPool() in GroupBuy.sol creates a new contribution pool around an NFT. It specifies a target _initialPrice as minimum amount of ETH the NFT will cost, and _totalSupply which is the number of Raes to be minted on purchase success.

minBidPrices is calculated from the two numbers. All future bids must be at least minBidPrices. It is assumed that if the totalSupply of Raes is filled up, the group will collect the initialPrice.

// Calculates minimum bid price based on initial price of NFT and desired total supply
minBidPrices[currentId] = _initialPrice / _totalSupply;

The issue is that division rounding error will make minBidPrices too low. Therefore, when all Raes are minted using minBidPrices price: minBidPrices[currentId] * _totalSupply != _initialPrice

Therefore, not enough money has been collected to fund the purchase. It can be assumed that most people will use minBidPrices to drive the price they will choose. Therefore, even after discovering that the Group has not raised enough after filling the supply pool, it will be very hard to get everyone to top up the contribution by a bit. This is because the settled price which is collected from all contributions is minReservePrices, which is always the minimum price deposited.

Code in contribute that updates minReservePrices:

// Updates minimum reserve price if filled quantity amount is greater than 0
if (filledQuantity > 0) minReservePrices[_poolId] = getMinPrice(_poolId);

The check in purchase() that we don't charge more than minReservePrices from each contribution:

if (_price > minReservePrices[_poolId] * filledQuantities[_poolId])
    revert InvalidPurchase();

We can see an important contract functionality is not working as expected which will impair NFT purchases.

Impact

GroupBuys that are completely filled still don't raise stated target amount

Tools Used

Manual audit

Recommended Mitigation Steps

Round the minBidPrices up, rather than down. It will ensure enough funds are collected.