Tessera - Versus contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

Both usages of call do not check if the transfer of ETH was succesful:

payable(msg.sender).call{value: contribution}("");
...
payable(msg.sender).call{value: balance}("");

This can become very problematic when the recipient is a smart contract that reverts (for instance, temporarily) in its receive function. Then, GroupBuy still assumes that this ETH was transferred out and sets the balance to 0 or deletes userContributions[_poolId][msg.sender], although no ETH was transferred. This leads to a loss of funds for the recipient.

Proof Of Concept

We assume that the recipient is a smart contract that performs some logic in its receive function. For instance, it can be a nice feature for some people to automatically convert all incoming ETH into another token using an AMM. However, it can happen that the used AMM has too little liquidity at the moment or the slippage of a swap would be too high, leading to a revert in the receing contract. In such a scenario, the GroupBuy contract still thinks that the call was succesful, leading to lost funds for the recipient.

Recommended Mitigation Steps

require that the call was succesful.

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/1e408ebc1c4fdcc72678ea7f21a94d38855ccc0b/src/modules/GroupBuy.sol#L242

Vulnerability details

Impact

The purchase function does not require that an NFT is bought for exactly minReservePrices[_poolId] * filledQuantities[_poolId], the price is only not allowed to be greater:

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

This makes sense because it is not sensible to pay more when the purchase also succeeds with a smaller amount. However, the logic within claim does assume that the NFT was bought for minReservePrices[_poolId]. It decreases from contribution the quantity times the reserve price for all bids:

contribution -= quantity * reservePrice;

Only the remaining amount is reimbursed to the user, which can lead to a loss of funds.

Proof Of Concept

Let's say that filledQuantities[_poolId] = 100 and minReservePrices[_poolId] (i.e., the lowest bid) was 1 ETH. However, it was possible to buy the NFT for only 50 ETH. When a user has contributed 20 * 1 ETH, he does not get anything back when calling claim, although only 10 ETH (0.5 ETH * 20) of his contributions were used to buy the NFT. The overall loss of funds for all contributors is 50 ETH.

Recommended Mitigation Steps

Set minReservePrices[_poolId] to _price / filledQuantities[_poolId] after a purchase.

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/1e408ebc1c4fdcc72678ea7f21a94d38855ccc0b/src/modules/GroupBuy.sol#L455 https://github.com/code-423n4/2022-12-tessera/blob/1e408ebc1c4fdcc72678ea7f21a94d38855ccc0b/src/modules/GroupBuy.sol#L478

Vulnerability details

Impact

The functions _verifyUnsuccessfulState and _verifySuccessfulState should always have a differing behavior with regards to reversion, i.e. when one does not revert, the other should revert. In one condition, this is not true. Namely, when we have pool.success == false and block.timestamp == pool.terminationPeriod, this check within _verifyUnsuccessfulState is false:

if (pool.success || block.timestamp > pool.terminationPeriod) revert InvalidState();

Similarly, this check within _verifySuccessfulState is also false:

if (!pool.success && block.timestamp < pool.terminationPeriod) revert InvalidState();

Because this breaks a fundamental invariant of the contract, there are probably multiple ways to exploit it. One way an attacker can exploit is by calling claim (to get his contribution back completely), bidding again with a higher value than his previous contributions (to get his contributions back again).

Proof Of Concept

Let's assume we are at timestamp pool.terminationPeriod. Attacker Charlie has performed the lowest bid with quantity 10 and price 1 ETH. He calls claim to get his 10 ETH back. Now, he calls contribute with a quantity of 10 and a price of 2 ETH. Because this bid is higher than his previous one (which was the lowest one), his pendingBalances is set to 10 ETH (for the deleted entries) and his userContributions is set to 20 ETH (for this new contribution). He can now call claim again to get back his 20 ETH in userContributions, but also the 10 ETH in pendingBalances. Like that, he has stolen 10 ETH (and could use this attack pattern to drain the whole contract).

Recommended Mitigation Steps

Change < in _verifySuccessfulState to <=.

Lines of code

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

Vulnerability details

Impact

In OptimisticListingSeaport.propose, pendingBalances is set to the collateral. The purpose of this is that the proposer of a previous proposal can withdraw his collateral afterwards. However, this is done on the storage variable proposedListing after the new listing is already set:

_setListing(proposedListing, msg.sender, _collateral, _pricePerToken, block.timestamp);

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

Because of that, it will actually set pendingBalances of the new proposer. Therefore, the old proposer loses his collateral and the new one can make proposals for free.

Proof Of Concept

--- a/test/seaport/OptimisticListingSeaport.t.sol
+++ b/test/seaport/OptimisticListingSeaport.t.sol
@@ -379,8 +379,11 @@ contract OptimisticListingSeaportTest is SeaportTestUtil {
     /// ===== LIST =====
     /// ================
     function testList(uint256 _collateral, uint256 _price) public {
         // setup
         testPropose(_collateral, _price);
+        assertEq(optimistic.pendingBalances(vault, bob), 0);
         _increaseTime(PROPOSAL_PERIOD);
         _collateral = _boundCollateral(_collateral, bobTokenBalance);
         _price = _boundPrice(_price);

This test fails and optimistic.pendingBalances(vault, bob) is equal to _collateral.

Recommended Mitigation Steps

Run pendingBalances[_vault][proposedListing.proposer] += proposedListing.collateral; before the _setListing call, in which case the above PoC no longer works.

Lines of code

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

Vulnerability details

Impact

The functions list and cash overwrite the current value instead of increasing it:

pendingBalances[_vault][activeListing.proposer] = activeListing.collateral;

pendingBalances[_vault][activeListing.proposer] = collateral;

This can be very problematic because the value can be non-zero at this point. Then, the previous pending balance of the user is lost, leading to a loss of funds for the user.

Proof Of Concept

Let's say Bob makes a proposal with collateral 100. Because Alice makes a proposal with a lower price, pendingBalances[_vault][address(Bob)] is set to 100. Later on, Bob makes another proposal with an even lower price and collateral 100 again. This proposal suceeds and someone calls cash. Because there are still 5 collateral tokens left, pendingBalances[_vault][address(Bob)] is set to 5, overwriting the previous value of 100. Therefore, these 100 tokens are lost and Bob cannot call withdrawCollateral to retrieve them.

Recommended Mitigation Steps

Increase the balances instead of overwriting them.

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/1e408ebc1c4fdcc72678ea7f21a94d38855ccc0b/src/modules/GroupBuy.sol#L162

Vulnerability details

Impact

The argument _market of GroupBuy.purchase is not validated. The following call is directly performed on it:

address vault = IMarketBuyer(_market).execute{value: _price}(_purchaseOrder);

Then, it is checked that the returned address owns the NFT:

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();
}

This can be exploited very easily: A user can create a contract that returns some address there and get the sent ETH or the NFT. To steal the ETH, he would simply return the current owner of the NFT as address. To steal the NFT, he would use the funds to buy the NFT and then return his address in the execute function.

Proof Of Concept

A malicious contract that exploits the described vulnerability to steal the ETH looks like this:

function execute(bytes memory _purchaseOrder) external payable {
	return IERC721(nftContract).ownerOf(tokenID);
}

Where nftContract and tokenID where previously set to match the ones of the caller.

The attacker then calls purchase with the maximum possible price and this contract as _market.

Recommended Mitigation Steps

Validate _market and only allow white-listed addresses there.

Lines of code

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

Vulnerability details

Impact

In GroupBuy.purchase, poolInfo[_poolId].success (which prevents buying the same NFT again) is only set to true after the sale was executed. This can be exploited by reentering in the following line:

address vault = IMarketBuyer(_market).execute{value: _price}(_purchaseOrder);

There are two sources of reentrancy there, the market itself (which may support callbacks) and the NFT transfer (with its onERC721Received hook). Depending on the used market, user-controlled (over _purchaseOrder, which is user-controlled) calls can be performed.

Proof Of Concept

Let's say that some NFT marketplace is used for buying the NFT. The calldata for that marketplace is arbitrary, as long as the vault is the recipient of the NFT in the end (which is a common pattern and also makes sense here). A malicious user submits a calldata that buys the NFT and transfers it to his smart contract. In the onERC721Received, he lists the NFT again and now calls GroupBuy.purchase with the calldata that buys the NFT and transfers it to the vault. In the end, both calls succeed, the attacker gained the _price amount and the group had to pay two times the _price (or potentially even more than that, as this attack can be executed multiple times).

Recommended Mitigation Steps

Set poolInfo[_poolId].success before the external call.

Lines of code

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

Vulnerability details

Impact

In GroupBuy.purchase, when no proof is provided, it is required that the provided token ID is equal to the stored merkleRoot:

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

This makes sense because the token ID is stored in the variable merkleRoot directly when only one ID is provided. However, the problem is that a user can also provide no merkle proof when merkleRoot does not contain the token ID of a single token, but a valid merkle root. In that case, the user can mint the token with the ID of the merkle root (interpreteted as a uint256), although that was never intended. Because token IDs are arbitrary uint256 and do not have to be consecutive, this can be a valid token ID.

Note on severity: The issue is very similar (or basically the same) as M-01 in the Seaport contest, where cmichel provided the following reasoning for the severity:

One might argue that this attack is not feasible because the provided hash is random and tokenIds are generally a counter. However, this is not required in the standard. “While some ERC-721 smart contracts may find it convenient to start with ID 0 and simply increment by one for each new NFT, callers SHALL NOT assume that ID numbers have any specific pattern to them, and MUST treat the ID as a ‘black box’.” EIP721 Neither do the standard OpenZeppelin/Solmate implementations use a counter. They only provide internal _mint(address to, uint256 id) functions that allow specifying an arbitrary id. NFT contracts could let the user choose the token ID to mint, especially contracts that do not have any linked off-chain metadata like Uniswap LP positions. Therefore, ERC721-compliant token contracts are vulnerable to this attack.

Proof Of Concept

We assume that the root of the merkle tree for some provided token IDs is 0xDEADBEEF.. When calling purchase, the user can now provide an empty _purchaseProof and use uint256(0xDEADBEEF..) as _tokenId. This allows him to buy a token ID that the creator never whitelisted (uint256(0xDEADBEEF..)).

Recommended Mitigation Steps

Also hash a single token ID before storing it in merkleRoot (and before the check in purchase).

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

GroupBuy.contribute does not set pendingBalances for unused capital, leading to locked up money

GroupBuy: DoS when someone made a lot of bids

GroupBuy.claim iterates over all bids that the caller has made. When this is a very large list, this iteration can use a lot of gas and it may result in situations where someone cannot call claim, leading to a loss of funds. Consider allowing partial claiming by specifying a range.

GroupBuy: Insertion timestamp ignored

The documentation states that "If the users have the same quantity as well, the bid that was placed later will have Raes removed.". However, with the current implementation, this is not always true. Within the priority queue, the insertion timestamp is not part of the ordering function isGreater and the queue implementation does not guarantee that a later inserted item will be before an earllier inserted one and vice-versa. This is the case because do not care about the exact position within a level of the binary heap, only about the relationship between the levels.

If this behavior should be enforced, the timestamp would need to be part of the ordering relationship.

GroupBuy.contribute does not set pendingBalances for unused capital, leading to locked up money

When there is some unfilled quantity within GroupBuy.contribute, the corresponding amount is not subtracted from userContributions / totalContributions and pendingBalances is not increased by the corresponding value. Because of that, the user has to wait until the purchase is finished such that he can call claim to get back this money. This is very capital-inefficient, as it is already clear at this point that the money that was sent for the unfilled quantity will never be used (and can never be used), so it would be preferable to allow the user to withdraw it immediately.

GroupBuy.printQueue: Debug function in production code

The function GroupBuy.printQueue is only intended for debugging (and does not do anything without log statements). Consider removing it before deployment to decrease the bytecode size (and therefore the deployment costs).

Incentive for NFT seller to call purchase with highest possible value

Because anyone can call GroupBuy.purchase, there is a large incentive for the seller of an NFT to call this function with the highest possible value (minReservePrices[_poolId] * filledQuantities[_poolId]), even if his NFT is listed for a much lower price. Consider restricting this function to only wallets that contributed to the buy (which have an incentive to buy the NFT cheaply).

OptimisticListingSeaport.updateFeeReceiver: Two-step process for changing fee receiver recommended

For actions like updateFeeReceiver that are only callable by the current address (current fee receiver in this case), a two step process for changing the addresses is recommended. Like that, it is ensured that the fee receiver is not lost irreversibly when an invalid address is provided.

OptimisticListingSeaport._constructOrder: Wrong comment

The following comment is written in _constructOrder:

// 1 Consideration for the listing itself + 1 consideration for the fees
orderParams.totalOriginalConsiderationItems = 3;

However, instead of one (like the comment says), there are two considerations for the fees (Tessera + OpenSea), resulting in 3 overall consideration items.

OptimisticListingSeaport.propose: activeListings[_vault] instead of storage variable activeListing used

In the function propose, activeListings[_vault] is used when comparing the price per token:

if (
            _pricePerToken >= proposedListing.pricePerToken ||
            _pricePerToken >= activeListings[_vault].pricePerToken
        ) revert NotLower();

However, a few lines above, this value is already loaded as a storage variable (together with proposedListing:

Listing storage proposedListing = proposedListings[_vault];
Listing storage activeListing = activeListings[_vault];

To save this mapping read, using activeListing is recommended there.

OptimisticListingSeaport.propose: Mismatch with documentation

According to the documentation, "The proposed listing price must be at least 5% cheaper than the active proposal price.". However, it is only checked if the price is lower than min(active proposal price, proposed proposal price), not if it is at least 5% lower:

if (
            _pricePerToken >= proposedListing.pricePerToken ||
            _pricePerToken >= activeListings[_vault].pricePerToken
        ) revert NotLower();

OptimisticListingSeaport: Calling rejectProposal and rejectActive with zero amount possible

It is possible to call these functions with an _amount of 0, even if there are no proposals for a vault. While this does not introduce any direct vulnerabilities, the corresponding events will still be emitted, which can be confusing for monitoring solutions. Consider validating that the amount is greater than 0.

OptimisticListingSeaport ignores royalties / creator fees

The listings that are created only include the Tessera and the OpenSea fee, but do not include any royalty payments, even if the NFT requires them. This could become quite problematic with OpenSea's royalty enforcement (see here, here, or here), where transfers can be blocked when royalties are ignored. It is not very clear to me how OpenSea will handle listings on Seaport that ignore royalties. Obviously, they will not completely block their own smart contract, but maybe they will start enforcing in a Seaport upgrade that royalties are enforced, even if the listing is done from another protocol (and not from the Opensea frontend, where the royalties seem to be enforced at the moment).

Because of that, it may be desirable to respect royalties.