Omniscia Seen Haus Audit

SaleBuilderFacet Manual Review Findings

SaleBuilderFacet Manual Review Findings

SBF-01M: Inexistent Sanitization of Sale Start

Description:

The documentation states that the function should revert if the _start of the sale is in the past, however, no such logical guarantee is enforced in the code.

Example:

contracts/market/handlers/facets/SaleBuilderFacet.sol
70* Reverts if:
71 * - Sale starts in the past
72 * - Sale exists for consignment
73 * - Consignment has already been marketed
74 *
75 * Emits a SalePending event.
76 *
77 * @param _consignmentId - id of the consignment being sold
78 * @param _start - the scheduled start time of the sale
79 * @param _price - the price of each item in the lot
80 * @param _perTxCap - the maximum amount that can be bought in a single transaction
81 * @param _audience - the initial audience that can participate. See {SeenTypes.Audience}
82 */
83function createPrimarySale (
84 uint256 _consignmentId,
85 uint256 _start,
86 uint256 _price,
87 uint256 _perTxCap,
88 Audience _audience
89)
90external
91override
92onlyRole(SELLER)
93onlyConsignor(_consignmentId)
94{
95 // Get Market Handler Storage slot
96 MarketHandlerLib.MarketHandlerStorage storage mhs = MarketHandlerLib.marketHandlerStorage();
97
98 // Fetch the consignment
99 Consignment memory consignment = getMarketController().getConsignment(_consignmentId);
100
101 // Make sure the consignment hasn't been marketed
102 require(consignment.marketHandler == MarketHandler.Unhandled, "Consignment has already been marketed");
103
104 // Get the storage location for the sale
105 Sale storage sale = mhs.sales[consignment.id];
106
107 // Make sure sale doesn't exist (start would always be non-zero on an actual sale)
108 require(sale.start == 0, "Sale exists");
109
110 // Set up the sale
111 setAudience(_consignmentId, _audience);
112 sale.consignmentId = _consignmentId;
113 sale.start = _start;

Recommendation:

We advise such a require statement to be introduced to the codebase. If the _start is accidentally set to 0 in the current case, the NFT will be permanently locked.

Alleviation:

The comment has been updated to ensure that a positive value for _start needs to be passed in and the code has been updated to reflect that via a require check.

SBF-02M: Inexistent Validation of Proper Supply

Description:

The ERC-721 workflow for creating a sale does not properly validate that the user-supplied _supply value is equal to 1 and can cause the system to misbehave.

Example:

contracts/market/handlers/facets/SaleBuilderFacet.sol
182// Make sure supply is non-zero
183require (_supply > 0, "Supply must be non-zero");
184
185// Make sure this contract is approved to transfer the token
186// N.B. The following will work because isApprovedForAll has the same signature on both IERC721 and IERC1155
187require(IERC1155Upgradeable(_tokenAddress).isApprovedForAll(msg.sender, address(this)), "Not approved to transfer seller's tokens");
188
189// To register the consignment, tokens must first be in MarketController's possession
190if (IERC165Upgradeable(_tokenAddress).supportsInterface(type(IERC1155Upgradeable).interfaceId)) {
191
192 // Ensure seller owns sufficient supply of token
193 require(IERC1155Upgradeable(_tokenAddress).balanceOf(msg.sender, _tokenId) >= _supply, "Seller has insufficient balance of token");
194
195 // Transfer supply to MarketController
196 IERC1155Upgradeable(_tokenAddress).safeTransferFrom(
197 msg.sender,
198 address(getMarketController()),
199 _tokenId,
200 _supply,
201 new bytes(0x0)
202 );
203
204} else {
205
206 // Token must be a single token NFT
207 require(IERC165Upgradeable(_tokenAddress).supportsInterface(type(IERC721Upgradeable).interfaceId), "Invalid token type");
208
209 // Transfer tokenId to MarketController
210 IERC721Upgradeable(_tokenAddress).safeTransferFrom(
211 msg.sender,
212 address(getMarketController()),
213 _tokenId
214 );
215
216}
217
218// Register consignment
219Consignment memory consignment = getMarketController().registerConsignment(Market.Secondary, msg.sender, _seller, _tokenAddress, _tokenId, _supply);

Recommendation:

We advise such sanitization to be introduced in the else code block of the ERC-721 transfer.

Alleviation:

A require check has been introduced ensuring that the _supply is equal to 1.