search

bosonprotocol / boson-protocol-contracts

Boson Protocol V2 (latest)
https://bosonprotocol.io/
GNU General Public License v3.0
32 stars 8 forks source link

pending AuthToken is not cleared during the updateSeller #682

Closed zajck closed 1 year ago

zajck commented 1 year ago

In updateSeller, pendingAddressUpdatesBySeller gets cleared every time before pending values are stored https://github.com/bosonprotocol/boson-protocol-contracts/blob/451dc3d0866ba6694821537803ba32939de34798/contracts/protocol/facets/SellerHandlerFacet.sol#L116 This ensures that pendingAddressUpdatesBySeller values are the latest, submitted by the seller, so they have a chance to override or delete the entries.

pendingAuthTokenUpdatesBySeller should be cleared the same way. If it's not, the following can happen:

  1. A seller calls updateSeller with some AuthToken.
  2. The seller decides, they don't want to finalize the update with this AuthToken and would like to remove it. They call updateSeller with non-zero admin address and AuthToken of type None.
  3. As the result, both pendingAddressUpdatesBySeller.admin and pendingAuthTokenUpdatesBySeller are filled with values which is not the intended behaviour https://github.com/bosonprotocol/boson-protocol-contracts/blob/451dc3d0866ba6694821537803ba32939de34798/contracts/protocol/facets/SellerHandlerFacet.sol#L123

Consequences

  1. The seller cannot abuse it to have both the admin address and auth token as the method of their authentication. They can opt-in to update by either (or even both) of them, but the protocol will always use only the last method they choose.
  2. The event SellerUpdatePending will have non zero values for both pendingSeller and pendingAuthToken which is not expected. Especially if the seller calls updateSeller without AuthToken they would expect pendingAuthToken to be zeroed.
  3. If the seller transfers auth token to someone else, they can claim control over the seller's account, which presents a security issue.
  4. The only way the seller can remove pendingAuthToken from the storage is to opt-in to update with that token. If they don't want to do it (or maybe even cannot do it), they cannot just use updateSeller to achieve that. The only thing they can do is update to another token, which is not likely to ever be owned by anyone.

Recommendation

Delete lookups.pendingAuthTokenUpdatesBySeller[_seller.id] before storing new values.

zajck commented 1 year ago

Fixed in #684