Omniscia Boson Protocol Audit

SellerHandlerFacet Manual Review Findings

SellerHandlerFacet Manual Review Findings

SHF-01M: Incorrect Iterator Usage

Description:

The nested for loop within the SellerHandlerFacet::optInToSellerUpdate function re-uses the i variable of the outer loop instead of declaring a new one. As such, the inner loop can corrupt the outer loop's iteration.

Impact:

If the SellerHandlerFacet::optInToSellerUpdate function is invoked with multiple _fieldsToUpdate, the iterator will potentially skip or repeat fields to update as it will jump from i to collectionCount, the latter of which can be greater than or lower than the expected value of i + 1.

Example:

contracts/protocol/facets/SellerHandlerFacet.sol
233for (uint256 i = 0; i < _fieldsToUpdate.length; i++) {
234    SellerUpdateFields role = _fieldsToUpdate[i];
235

236    // Approve admin update
237    if (role == SellerUpdateFields.Admin && sellerPendingUpdate.admin != address(0)) {
238        require(sellerPendingUpdate.admin == sender, UNAUTHORIZED_CALLER_UPDATE);
239

240        preUpdateSellerCheck(_sellerId, sender, lookups);
241

242        // Delete old seller id by admin mapping
243        delete lookups.sellerIdByAdmin[seller.admin];
244

245        // Update admin
246        seller.admin = sender;
247

248        // Store new seller id by admin mapping
249        lookups.sellerIdByAdmin[sender] = _sellerId;
250

251        // Delete pending update admin
252        delete sellerPendingUpdate.admin;
253

254        // Delete auth token for seller id if it exists
255        if (authToken.tokenType != AuthTokenType.None) {
256            delete lookups.sellerIdByAuthToken[authToken.tokenType][authToken.tokenId];
257            delete protocolEntities().authTokens[_sellerId];
258        }
259

260        updateApplied = true;
261    } else if (role == SellerUpdateFields.Assistant && sellerPendingUpdate.assistant != address(0)) {
262        // Approve assistant update
263        require(sellerPendingUpdate.assistant == sender, UNAUTHORIZED_CALLER_UPDATE);
264

265        preUpdateSellerCheck(_sellerId, sender, lookups);
266

267        // Delete old seller id by assistant mapping
268        delete lookups.sellerIdByAssistant[seller.assistant];
269

270        // Update assistant
271        seller.assistant = sender;
272

273        // Transfer ownership of voucher contract to new assistant
274        IBosonVoucher(lookups.cloneAddress[_sellerId]).transferOwnership(sender); // default voucher contract
275        Collection[] storage sellersAdditionalCollections = lookups.additionalCollections[_sellerId];
276        uint256 collectionCount = sellersAdditionalCollections.length;
277        for (i = 0; i < collectionCount; i++) {
278            // Additional collections (if they exist)
279            IBosonVoucher(sellersAdditionalCollections[i].collectionAddress).transferOwnership(sender);
280        }

Recommendation:

We advise a new iterator to be declared (i.e. j) to ensure that the loops do not affect each other's iterators.

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The inner loop iterator has been correctly updated to a new variable labelled j, alleviating this exhibit.

Prev

SellerBase.sol (SBE-M)

Next

BosonVoucher.sol (BVR-C)

On this page