BosonVoucher Manual Review Findings

BVR-01M: Inexistent Restriction of Approval for Owner

Type	Severity	Location
Logical Fault		BosonVoucher.sol:L558, L561

Description:

The Boson Protocol PR#571 and specifically this thread indicates that the _operator being approved should not be the OwnableUpgradeable::owner, however, the function reverts solely if the _operator is the address(this) value.

Example:

Copy
558function setApprovalForAllToContract(address _operator, bool _approved) external onlyOwner {
559    require(_operator != address(0), INVALID_ADDRESS);
560
561    _setApprovalForAll(address(this), _operator, _approved);
562}

Recommendation:

We advise the Boson Protocol to revisit this particular change and ensure that the owner of the referenced comment refers to the "owner" of the EIP-721 vouchers rather than the OwnableUpgradeable::owner.

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The Boson Protocol team clarified that the owner in question is the voucher's owner and as such, the code behaves as expected rendering this exhibit nullified.

BVR-02M: Potentially Malformed Contract Storage Space

Type	Severity	Location
Language Specific		BosonVoucher.sol:L833

Description:

The ERC2771ContextUpgradeable contract implementation used to reserve one storage slot in its original v4.0.0 OpenZeppelin implementation and up to v4.4.0, however, it was updated to make use of immutable variables in v4.5.0 and up thus causing it to no longer require a storage space.

While the dependency itself maintains a proper storage structure due to the usage of a __gap value that was updated, the BosonVoucherBase does not have such a gap and adjusts its storage space between implementations.

Impact:

Given that this vulnerability would solely arise if ERC2771ContextUpgradeable was updated by OpenZeppelin to make use of storage slots and / or if the Boson Protocol inherits implementations that make use of storage space for the BosonVoucher, its severity is indeterminate as it may never manifest in practice.

Example:

Copy
783/*
784 * Returns storage pointer to location of private variables
785 * 0x99 is location of _owners
786 * 0x9a is location of _balances
787 *
788 * Since ERC721UpgradeableStorage slot is 0x99
789 * _owners slot is ERC721UpgradeableStorage + 0
790 * _balances slot is ERC721UpgradeableStorage + 1
791 */

Recommendation:

We advise the __gap value that was removed from BosonVoucher to be relocated to the BosonVoucherBase implementation, ensuring the storage space of BosonVoucher is unaffected between upgrades of the BosonVoucherBase dependency.

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The BosonVoucherBase already possessed a __gap variable, rendering this exhibit nullified.

BVR-03M: Inexistent Access Control of Protocol Withdrawals

Type	Severity	Location
Logical Fault		BosonVoucher.sol:L766

Description:

The BosonVoucherBase::withdrawToProtocol function does not apply any access control to its caller, permitting anyone to invoke it and thus cause funds from the contract to be deposited to the protocol.

While the funds will still be owned by the correct sellerId, the BosonVoucherBase contract is capable of being the "purchaser" of a conditional offer in the Boson Protocol system. These conditional offers can impose restrictions based on EIP-20 asset balances that can be compromised by this function in an on-chain race condition.

Impact:

It is presently possible to hijack threshold-based commit authorizations that are performed by the BosonVoucherBase by invoking its BosonVoucherBase::withdrawToProtocol function.

Example:

Copy
766function withdrawToProtocol(address[] calldata _tokenList) external {
767    address protocolDiamond = IClientExternalAddresses(BeaconClientLib._beacon()).getProtocolAddress();
768    uint256 sellerId = getSellerId();
769
770    for (uint256 i = 0; i < _tokenList.length; i++) {
771        address token = _tokenList[i];
772        if (token == address(0)) {
773            uint256 balance = address(this).balance;
774            IBosonFundsHandler(protocolDiamond).depositFunds{ value: balance }(sellerId, token, balance);
775        } else {
776            uint256 balance = IERC20(token).balanceOf(address(this));
777            IERC20(token).approve(protocolDiamond, balance);
778            IBosonFundsHandler(protocolDiamond).depositFunds(sellerId, token, balance);
779        }
780    }
781}

Recommendation:

We advise the code to apply proper access control and ensure that the function can only be called by the OwnableUpgradeable::owner of the contract.

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The Boson Protocol team evaluated this exhibit and acknowledged that it is an edge condition, however, they do not plan to remediate it as they wish withdrawals to be flexible.

BVR-04M: Inexistent Legacy Compatibility of Boson Voucher Premints

Type	Severity	Location
Logical Fault		BosonVoucher.sol:L452, L706, L736

Description:

The updated BosonVoucher implementation utilizes a new identification system for tokens whereby they are composed of the offer ID as well as exchange ID in the upper and lower halves of the 256 bit number respectively.

Preminted vouchers that had been issued before the update, however, will misbehave as they will not be able to be committed properly and will yield incorrect BosonVoucher::tokenURI values.

Impact:

Any BosonVoucher beacon implementations that were upgraded and had lingering preminted vouchers will fail to function as expected after the upgrade.

Example:

Copy
693function _beforeTokenTransfer(address _from, address _to, uint256 _tokenId, uint256) internal override {
694    // Derive the exchange id
695    uint256 exchangeId = _tokenId & type(uint128).max;
696    if (_isCommitable) {
697        // If is committable, invoke commitToPreMintedOffer on the protocol
698
699        // Set _isCommitable to false
700        _isCommitable = false;
701
702        // Set the preminted token as committed
703        _committed[_tokenId] = true;
704
705        // Derive the offer id
706        uint256 offerId = _tokenId >> 128;
707
708        // If this is a transfer of preminted token, treat it differently
709        address protocolDiamond = IClientExternalAddresses(BeaconClientLib._beacon()).getProtocolAddress();
710        IBosonExchangeHandler(protocolDiamond).commitToPreMintedOffer(payable(_to), offerId, exchangeId);
711    } else if (_from != address(0) && _to != address(0) && _from != _to) {

Recommendation:

We advise the BosonVoucher system to either "delete" all previously issued premints as part of its upgrade or to support them properly, the latter of which we advise given that preminted offers are closely intertwined with offers and the OrchestrationHandlerFacet1.

Alleviation (2b9f60b6c3):

The Boson Protocol team stated that they deem this exhibit incorrect as the new identification system was introduced in tandem with the premint functionality of vouchers.

We consider this to be incorrect as the audit we performed of v2.2.0-rc.2 contained a preminted system without the new identification system in place.

We advise the Boson Protocol team to re-evaluate this exhibit and specify what version of v2.2.0 was actively deployed and will be replaced by the currently-audited version.

Alleviation (584e7d054c):

The Boson Protocol team clarified that the Polygon mainnet deployment has been upgraded from v2.2.0 to v2.2.1 with no intermediate steps and as such, no lingering legacy vouchers are present in the system.

Based on this fact, we consider the exhibit nullified as it arose from a version that was never actively deployed.

BVR-05M: Inexistent Transfer of Preminted Voucher Ranges

Type	Severity	Location
Logical Fault		BosonVoucher.sol:L175, L227, L291, L483-L489

Description:

The process of issuing a range reservation within BosonVoucherBase::reserveRange ensures that the _to address is either the contract itself or the contract's owner, however, this assumption may have been invalidated when BosonVoucherBase::preMint has been invoked as the transfer of ownership of a voucher (BosonVoucherBase::transferOwnership) does not update the range owner values.

Impact:

The current mechanism permits the owner of a voucher to reserve a large range and then transfer their voucher. When doing so, the new owner will be unable to utilize this reserved range for themselves as the owner would have been cached to the previous owner, a behaviour we consider incorrect.

Example:

Copy
152function reserveRange(uint256 _offerId, uint256 _start, uint256 _length, address _to) external onlyRole(PROTOCOL) {
153    // _to must be the contract address or the contract owner (operator)
154    require(_to == address(this) || _to == owner(), INVALID_TO_ADDRESS);
155
156    // Prevent reservation of an empty range
157    require(_length > 0, INVALID_RANGE_LENGTH);
158
159    // Adjust start id to include offer id
160    require(_start > 0, INVALID_RANGE_START);
161    _start += (_offerId << 128);
162
163    // Prevent overflow in issueVoucher and preMint
164    require(_start <= type(uint256).max - _length, INVALID_RANGE_LENGTH);
165
166    // Get storage slot for the range
167    Range storage range = _rangeByOfferId[_offerId];
168
169    // Revert if the offer id is already associated with a range
170    require(range.length == 0, OFFER_RANGE_ALREADY_RESERVED);
171
172    // Store the reserved range
173    range.start = _start;
174    range.length = _length;
175    range.owner = _to;
176
177    emit RangeReserved(_offerId, range);
178}
179
180/**
181 * @notice Pre-mints all or part of an offer's reserved vouchers.
182 *
183 * For small offer quantities, this method may only need to be
184 * called once.
185 *
186 * But, if the range is large, e.g., 10k vouchers, block gas limit
187 * could cause the transaction to fail. Thus, in order to support
188 * a batched approach to pre-minting an offer's vouchers,
189 * this method can be called multiple times, until the whole
190 * range is minted.
191 *
192 * A benefit to the batched approach is that the entire reserved
193 * range for an offer need not be pre-minted at one time. A seller
194 * could just mint batches periodically, controlling the amount
195 * that are available on the market at any given time, e.g.,
196 * creating a pre-minted offer with a validity period of one year,
197 * causing the token range to be reserved, but only pre-minting
198 * a certain amount monthly.
199 *
200 * Caller must be contract owner (seller assistant address).
201 *
202 * Reverts if:
203 * - Offer id is not associated with a range
204 * - Amount to mint is more than remaining un-minted in range
205 * - Offer already expired
206 * - Offer is voided
207 *
208 * @param _offerId - the id of the offer
209 * @param _amount - the amount to mint
210 */
211function preMint(uint256 _offerId, uint256 _amount) external onlyOwner {
212    // Get the offer's range
213    Range storage range = _rangeByOfferId[_offerId];
214
215    // Revert if id not associated with a range
216    require(range.length != 0, NO_RESERVED_RANGE_FOR_OFFER);
217
218    // Revert if no more to mint in range
219    require(range.length >= range.minted + _amount, INVALID_AMOUNT_TO_MINT);
220
221    // Make sure that offer is not expired or voided
222    (Offer memory offer, OfferDates memory offerDates) = getBosonOffer(_offerId);
223    require(!offer.voided && (block.timestamp <= offerDates.validUntil), OFFER_EXPIRED_OR_VOIDED);
224
225    // Get the first token to mint
226    uint256 start = range.start + range.minted;
227    address to = range.owner;
228
229    // Pre-mint the range
230    uint256 tokenId;
231    for (uint256 i = 0; i < _amount; i++) {
232        tokenId = start + i;
233
234        emit Transfer(address(0), to, tokenId);
235    }
236
237    // Bump the minted count
238    range.minted += _amount;
239
240    // Update to total balance
241    getERC721UpgradeableStorage()._balances[to] += _amount;
242
243    emit VouchersPreMinted(_offerId, start, tokenId);
244}

Recommendation:

This issue is slightly complex to solve as preminted offer burns should be done on the address that was consumed during a BosonVoucherBase::preMint call. As such, we propose a uniform range "ownership" mechanism as follows:

• The BosonVoucherBase::reserveRange function will assign a bool value on the Range struct that indicates whether it is a contract-or-owner ownership style range.
• The BosonVoucherBase::preMint function will evaluate either the address(this) value or the OwnableUpgradeable::owner of the contract depending on the aforementioned bool variable. The result will be the recipient of the premint and will additionally be stored on the Range struct.
• The BosonVoucherBase::burnPremintedVouchers will rely on the newly stored recipient of the premint in the previous step rather than a dynamic evaluation, ensuring that the preminted vouchers are burned from the correct party.

These adjustments to the four premint-related functions are sufficient to alleviate the issue described.

Alleviation (2b9f60b6c3):

The Boson Protocol team stated that they wish to remain compatible with the EIP-721 standard in full and emit the correct Transfer events whenever a premint occurs. As such, they consider the current approach a deliberate choice and wish to acknowledge this exhibit.

We would like to state that this issue should be re-visited by the Boson Protocol team and lead to a potential refactor in the way premints work in the codebase as we consider it a flaw worthy of being remediated due to the potential user-experience hinderances it may cause.

Alleviation (584e7d054c):

After re-evaluating the exhibit, the Boson Protocol has retained their decision to proceed with the current approach for premint operations.

To ensure users of the range functionality are aware of how it behaves, extensive documentation and warnings will be placed in the codebase to note the feature is experimental and detail the caveats that arise from utilizing it.

Based on these actions, we consider this exhibit acknowledged and advise it to be monitored by the Boson Protocol team in future releases of the protocol.

BVR-06M: Insufficient Protection of Contract Assets

Type	Severity	Location
Logical Fault		BosonVoucher.sol:L536-L543

Description:

The BosonVoucherBase::callExternalContract is insecure as it permits the contract to perform arbitrary calls, potentially compromising the assets it is in possession of. A short list of disallowed selector values has been specified, however, it is insufficient if the contract is expected to not reduce its balance in any way.

Impact:

It is presently possible to compromise funds held within the BosonVoucher despite the security measures in place within BosonVoucherBase::callExternalContract.

Example:

Copy
531function callExternalContract(address _to, bytes calldata _data) external payable onlyOwner returns (bytes memory) {
532    require(_to != address(0), INVALID_ADDRESS);
533
534    // Prevent invocation of functions that would allow transfer of tokens from this contract
535    bytes4 selector = bytes4(_data[:4]);
536    require(
537        selector != IERC20.transfer.selector &&
538            selector != IERC20.approve.selector &&
539            selector != IERC20.transferFrom.selector &&
540            selector != DAI.push.selector &&
541            selector != DAI.move.selector,
542        FUNCTION_NOT_ALLOWLISTED
543    );
544
545    return _to.functionCallWithValue(_data, msg.value, FUNCTION_CALL_NOT_SUCCESSFUL);
546}

Recommendation:

We advise either the selector list to be made a mapping that can be maintained by the Boson Protocol team as the current solution is insufficient in preventing token transfers.

As tangible examples, an ERC20::burn call can be made on the DAI token, an ERC20Like::increaseAllowance call can be made on the USDC token, and ERC20Like::increaseApproval as well as ERC20Like::increaseAllowance functions exist on multiple tokens.

Depending on the intended purpose of the BosonVoucherBase::callExternalContract function, multiple robust solutions can be employed. We will detail a few approaches in brief that the Boson Protocol can follow below:

• Implement a "target" whitelist in case BosonVoucher instances are expected to interact with a small subset of contracts (i.e. Seaport based on the PR of the changes).
• Implement a blanket asset protection measure by evaluating whether the _to address supports the ERC20::balanceOf function. As almost all EIP-20 tokens require direct interaction to transfer tokens or approve, a successful ERC20::balanceOf invocation would indicate the target _to address is a token and should not be interacted with.
• Maintain a protocol-wide selector blacklist that uses a mapping entry that can be updated in the future as more signatures need to be blacklisted.

To note, the contract seems to solely protect EIP-20 assets. If EIP-721 or EIP-1155 assets need to be protected as well (which we believe to be the case), all the aforementioned solutions would need to be adapted accordingly to support those two standards on top (i.e. ERC721::ownerOf instead of ERC20::balanceOf).

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The Boson Protocol proceeded with applying the second approach we detailed whereby an IERC20::balanceOf invocation is attempted on the target _to address and if it succeeds the interaction is prohibited.

The Boson Protocol team stated that they do not wish to support EIP-721 / EIP-1155 asset protection at this stage, however, it is something that they will keep track of for a potential future implementation.

Given that EIP-20 assets are adequately protected via the remediation introduced, we consider this exhibit to be fully alleviated.

BVR-07M: Storage Conflict of Beacon Implementation

Type	Severity	Location
Language Specific		BosonVoucher.sol:L51-L54

Description:

The BosonVoucherBase contract is meant to be the logic target of beacon implementations per its documentation as well as the project's code structure, however, in the latest update the _rangeOfferIds entry was omitted thereby shifting its storage space upwards by one 32-byte slot and thus rendering the contract an incompatible upgrade for existing beacons.

Impact:

If the updated BosonVoucherBase was utilized as a beacon upgrade of the existing Boson Protocol, the storage space of all deployed vouchers would be corrupted causing significant unintended side effects.

Example:

Copy
50// Map an offerId to a Range for pre-minted offers
51mapping(uint256 => Range) private _rangeByOfferId;
52
53// Premint status, used only temporarly in transfers
54bool private _isCommitable;

Recommendation:

We advise the code to retain the _rangeOfferIds as a private variable that is aptly renamed to _rangeOfferIds_deprecated so as to indicate that it solely exists to retain the storage offset of the overall system.

Alleviation (2b9f60b6c3323fd234b570089ceff924cdb5851c):

The Boson Protocol team stated that the _rangeOfferIds variable was present in a release candidate that never made it to production.

As such, we consider this exhibit nullified given that the storage layout of the live contracts is unaffected by the referenced discrepancy.