Omniscia Boson Protocol Audit

SellerBase Manual Review Findings

SellerBase Manual Review Findings

SBE-01M: Potential of Race Condition in Seller Creation

TypeSeverityLocation
Logical FaultSellerBase.sol:L32-L86

Description:

The createSellerInternal function as well as where it is called apply no access control to the caller. Given that the seller creation system strictly associates user types with a single seller ID, it is possible for a race condition to manifest by detecting the creation of a seller account and adjusting the treasury address to a different one causing an unsuspecting user to never receive the funds they are owed for a sale.

Example:

contracts/protocol/bases/SellerBase.sol
32function createSellerInternal(
33 Seller memory _seller,
34 AuthToken calldata _authToken,
35 VoucherInitValues calldata _voucherInitValues
36) internal {
37 //Check active is not set to false
38 require(_seller.active, MUST_BE_ACTIVE);
39
40 //Admin address or AuthToken data must be present. A seller can have one or the other
41 require(
42 (_seller.admin == address(0) && _authToken.tokenType != AuthTokenType.None) ||
43 (_seller.admin != address(0) && _authToken.tokenType == AuthTokenType.None),
44 ADMIN_OR_AUTH_TOKEN
45 );
46
47 //Check that the addresses are unique to one seller Id, accross all roles. These addresses should always be checked. Treasury is not checked
48 require(
49 protocolLookups().sellerIdByOperator[_seller.operator] == 0 &&
50 protocolLookups().sellerIdByOperator[_seller.clerk] == 0 &&
51 protocolLookups().sellerIdByAdmin[_seller.operator] == 0 &&
52 protocolLookups().sellerIdByAdmin[_seller.clerk] == 0 &&
53 protocolLookups().sellerIdByClerk[_seller.operator] == 0 &&
54 protocolLookups().sellerIdByClerk[_seller.clerk] == 0,
55 SELLER_ADDRESS_MUST_BE_UNIQUE
56 );
57
58 //Do other uniqueness checks based on auth type
59 if (_seller.admin == address(0)) {
60 //Check that auth token is unique to this seller
61 require(
62 protocolLookups().sellerIdByAuthToken[_authToken.tokenType][_authToken.tokenId] == 0,
63 AUTH_TOKEN_MUST_BE_UNIQUE
64 );
65 } else {
66 //check that the admin address is unique to one seller Id, accross all roles
67 require(
68 protocolLookups().sellerIdByOperator[_seller.admin] == 0 &&
69 protocolLookups().sellerIdByAdmin[_seller.admin] == 0 &&
70 protocolLookups().sellerIdByClerk[_seller.admin] == 0,
71 SELLER_ADDRESS_MUST_BE_UNIQUE
72 );
73 }
74
75 // Get the next account Id and increment the counter
76 uint256 sellerId = protocolCounters().nextAccountId++;
77 _seller.id = sellerId;
78 storeSeller(_seller, _authToken);
79
80 // create clone and store its address cloneAddress
81 address voucherCloneAddress = cloneBosonVoucher(sellerId, _seller.operator, _voucherInitValues);
82 protocolLookups().cloneAddress[sellerId] = voucherCloneAddress;
83
84 // Notify watchers of state change
85 emit SellerCreated(sellerId, _seller, voucherCloneAddress, _authToken, msgSender());
86}

Recommendation:

We advise the seller creation flow to apply some form of access control to the caller or sanitization of the input arguments to ensure that an account cannot be created for other users and thus disallowing this type of fraud attempt from ever being feasible on-chain.

Alleviation (68ebb15f14b12c7e64c233ff00363398a127a71e):

The createSellerInternal function now mandates that the _seller.operator and _seller.clerk values (as well as, conditionally, the _seller.admin value) all represent the current transaction sender thus preventing accounts to be created for other users. To ensure the system still permits an account to be updated in relation to those values, an opt-in system has been introduced to the SellerHandlerFacet implementation that treats updates as "pending" until the recipient of the role "opts-in" to the upgrade manually, ensuring accounts and responsibilities cannot be offloaded to other addresses without confirmation of receipt. As a result of this workflow adjustment, we consider this exhibit alleviated in full.