Omniscia Nexera Protocol Audit

GenericSingleDatasetSubscriptionManager Manual Review Findings

GenericSingleDatasetSubscriptionManager Manual Review Findings

GSD-01M: Incorrect Restriction of Subscription Extension

Description:

A GenericSingleDatasetSubscriptionManager::_extendSubscription operation permits a subscription to be extended beyond the 365 day limit a subscription duration should abide by.

In detail, it can exceed the 365 day limit by a maximum of 30 days, leading to the actual maximum duration achievable as being 395 days.

Impact:

A subscription's duration can presently exceed the 365 limit enforced by its creation via the extension mechanism the contract exposes.

Example:

contracts/subscription/GenericSingleDatasetSubscriptionManager.sol
297function _extendSubscription(uint256 subscription, uint256 extraDurationInDays, uint256 extraConsumers) internal {
298 _requireMinted(subscription);
299
300 if (extraDurationInDays > 365) revert SUBSCRIPTION_DURATION_INVALID(1, 365, extraDurationInDays);
301
302 SubscriptionDetails storage sd = _subscriptions[subscription];
303 uint256 newDurationInDays;
304 uint256 newValidSince;
305 uint256 currentFee;
306
307 if (sd.validTill > block.timestamp) {
308 // Subscription is still valid but remaining duration must be <= 30 days to extend it
309 if (extraDurationInDays > 0)
310 if ((sd.validTill - block.timestamp) > 30 * 1 days)
311 revert SUBSCRIPTION_REMAINING_DURATION(30 * 1 days, (sd.validTill - block.timestamp));
312
313 // (sd.validTill - sd.validSince) was enforced during subscription to be an integral multiple of a day in seconds
314 uint256 currentDurationInDays = (sd.validTill - sd.validSince) / 1 days;
315 (, currentFee) = _calculateFee(currentDurationInDays, sd.paidConsumers);
316 newValidSince = sd.validSince;
317 newDurationInDays = currentDurationInDays + extraDurationInDays;
318 } else {
319 // Subscription is already invalid
320 // currentFee = 0;
321 newValidSince = block.timestamp;
322 newDurationInDays = extraDurationInDays;
323 }
324 uint256 newConsumers = sd.paidConsumers + extraConsumers;
325 (, uint256 newFee) = _calculateFee(newDurationInDays, newConsumers);
326 if (newFee <= currentFee) revert NOTHING_TO_PAY();
327
328 _charge(_msgSender(), newFee - currentFee);
329
330 sd.validSince = newValidSince;
331 sd.validTill = newValidSince + (newDurationInDays * 1 days);
332 sd.paidConsumers = newConsumers;
333 emit SubscriptionPaid(subscription, sd.validSince, sd.validTill, sd.paidConsumers);
334}

Recommendation:

We advise the extension code of GenericSingleDatasetSubscriptionManager::_extendSubscription to validate the newDurationInDays value rather than the extraDurationInDays value when enforcing the subscription duration limit, ensuring that a subscription can at most have a duration of 365 days.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

The referenced if conditional that would validate the extraDurationInDays value was relocated after the if-else block, validating that the newDurationInDays is not greater than the MAX_SUBSCRIPTION_DURATION_IN_DAYS (365) thus alleviating this exhibit as the duration of a subscription can no longer exceed the contract-enforced maximum.

GSD-02M: Inexistent Surcharge Protection of Subscriber

Description:

The GenericSingleDatasetSubscriptionManager::_subscribe and GenericSingleDatasetSubscriptionManager::_extendSubscription functions do not permit the caller to control how much funds they are charged for a particular subscription, permitting a malicious subscription manager to arbitrarily charge them.

In detail, the ERC20SubscriptionManager::setFee function permits the fee to be arbitrarily set by the data-set owner leading to an on-chain race condition whereby a data-set owner detects a subscription and alters the fee of the subscription with a higher-priority transaction.

Impact:

The vulnerability is trivial to exploit by creating a seemingly lucrative data-set whose price is consequentially altered maliciously the moment a subscription is detected, leading to "loss of funds" for the subscriber.

Example:

contracts/subscription/GenericSingleDatasetSubscriptionManager.sol
269function _subscribe(uint256 ds, uint256 durationInDays, uint256 consumers) internal returns (uint256 sid) {
270 _requireCorrectDataset(ds);
271 if (balanceOf(_msgSender()) != 0) revert CONSUMER_ALREADY_SUBSCRIBED(_msgSender());
272 if (durationInDays == 0 || durationInDays > 365) revert SUBSCRIPTION_DURATION_INVALID(1, 365, durationInDays);
273
274 if (consumers == 0) revert CONSUMER_ZERO();
275
276 (, uint256 fee) = _calculateFee(durationInDays, consumers);
277 _charge(_msgSender(), fee);
278
279 sid = ++_mintCounter;
280 SubscriptionDetails storage sd = _subscriptions[sid];
281 sd.validSince = block.timestamp;
282 sd.validTill = block.timestamp + (durationInDays * 1 days);
283 sd.paidConsumers = consumers;
284 _safeMint(_msgSender(), sid);
285 emit SubscriptionPaid(sid, sd.validSince, sd.validTill, sd.paidConsumers);
286}

Recommendation:

We advise the higher-level GenericSingleDatasetSubscriptionManager::subscribe and GenericSingleDatasetSubscriptionManager::extendSubscription functions to accept an additional argument that permits the subscriber to enforce an upper-limit on the amount they are willing to be charged for the particular operation, preventing the aforementioned race condition from manifesting.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

Both the GenericSingleDatasetSubscriptionManager::_subscribe and the GenericSingleDatasetSubscriptionManager::_extendSubscription functions have been updated to accept a maxFee and a maxExtraFee argument respectively which is passed in by the relevant top-level calls, addressing this exhibit in full.

GSD-03M: Incorrect Dependency Set

Description:

The GenericSingleDatasetSubscriptionManager contract is meant to be an upgradeable contract, however, it utilizes non-upgradeable variants of its OpenZeppelin dependency tree.

As a result, derivative contracts such as ERC20SubscriptionManager are unable to initialize the ERC721Enumerable implementation properly.

Impact:

The ERC20SubscriptionManager deployment will contain an empty EIP-721 name and symbol as its dependency is initialized solely in the logic implementation and not in the upgradeable implementation.

Example:

contracts/subscription/GenericSingleDatasetSubscriptionManager.sol
4import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
5import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
6import {ERC721Enumerable} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
7import {ISubscriptionManager} from "../interfaces/ISubscriptionManager.sol";
8import {IDatasetNFT} from "../interfaces/IDatasetNFT.sol";
9
10/**
11 * @title GenericSingleDatasetSubscriptionManager contract
12 * @author Data Tunnel
13 * @notice Abstract contract serving as the foundation for managing single Dataset subscriptions and related operations.
14 * Derived contracts mint ERC721 tokens that represent subscriptions to the managed Dataset, thus, subscriptions
15 * have unique IDs which are the respective minted ERC721 tokens' IDs.
16 * @dev Extends ISubscriptionManager, Initializable, ERC721Enumerable
17 */
18abstract contract GenericSingleDatasetSubscriptionManager is ISubscriptionManager, Initializable, ERC721Enumerable {

Recommendation:

We advise the code as a whole to make user of upgradeable OpenZeppelin contracts, ensuring that the ERC20SubscriptionManager can properly initialize the ERC721Enumerable contract in its ERC20SubscriptionManager::initialize function.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

The code of both the ERC20SubscriptionManager and the GenericSingleDatasetSubscriptionManager contracts has been updated to properly initialize the upgradeable variant of the ERC721Enumerable contract and to properly inherit the ERC721EnumerableUpgradeable contract from the relevant OpenZeppelin dependency respectively.

As a result, we consider this exhibit alleviated as correct dependencies and initializations occur throughout the two referenced contracts.