GenericSingleDatasetSubscriptionManager Manual Review Findings

GSD-01M: Incorrect Restriction of Subscription Extension

Type	Severity	Location
Logical Fault		GenericSingleDatasetSubscriptionManager.sol:L300, L317

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:

Copy
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

Type	Severity	Location
Logical Fault		GenericSingleDatasetSubscriptionManager.sol:L277, L328

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:

Copy
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

Type	Severity	Location
Logical Fault		GenericSingleDatasetSubscriptionManager.sol:L6, L18

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:

Copy
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.