The Graph A-1

The subscribe() function mistakenly compares the user subscription end date against block.number instead of block.timestamp. This prevents a user X from calling subscribe() for a user Y if user Y has an expired subscription.

Consider updating block.number to block.timestamp.

In extendSubscription(), the user’s current subscription is used to calculate how many additional tokens are required to extend the subscription up to the specified end.

However, a user may be able to get the extender to pay more than they intend. For example:

1. User X has a subscription from time 200 to 500 with rate 7. Current time is 230.
2. Extender calls extendSubscription() with user X’s address and an end time of 800.
3. User X front-runs transaction with:
   1. unsubscribe(), also getting a refund
   2. subscribe() with start as now, with end as a small amount after (such as 315), and with rate as high as they can afford.
4. Extender, intending to pay for 250 seconds of time, is now paying for 435 seconds, and at a much higher rate than intended.

This issue affects extenders linearly with approval size.

Consider updating extendSubscription’s parameters to include a intendedRate so the extender’s paid rate cannot be front-run.

Also consider replacing end with secondsToExtend so extenders cannot pay for more time than they intend.

A use case supported by the Subscriptions contract is allowing a third party to call fulfil() on a user’s pending subscription, converting the pending into an actual subscription and paying for it.

If the targeted user already has an existing subscription, unsubscribe() is called to refund the user before converting the pending subscription.

However, the unsubscribe() function only refunds msg.sender, which is different than the user in this use case. This causes one of the following issues:

• If the msg.sender has an active subscription, the msg.sender’s subscription is unsubscribed, and the target user’s subscription is overwritten, losing access to those funds.
• If the msg.sender does not have an active subscription, the call reverts with no active subscription, and the use case is blocked from functioning.

Note that the subscription status of either party can be freely switched from no-subscription to active-subscription by way of subscribe(), allowing a griefer to set up these scenarios at low cost.

Consider implementing an internal function _unsubscribe(address) which accepts an address, and calling it from unsubscribe() with msg.sender, and from _subscribe() with user.

The subscribe() function allows start and end values to have a maximum value of type(uint64).max, but the start value can be truncated during cast to int64 on lines 361-362 of unlocked():

uint256 len = uint256(
    SignedMath.max(
        0,
        **int256(int64(_subEnd)) -
            int256(Math.max(block.timestamp, _subStart))**
    )
);

As a result, unsubscribe() can mis-calculate the refund and leave a portion of funds locked in the contract.

Consider restricting subscription start and end to a maximum value of type(int64).max.

When user X calls subscribe() to create a subscription for user Y, a malicious user Z can front-run the transaction by calling subscribe() themselves with a long end and a minimal rate.

This can disrupt the [assumingly less-common] use case of a machine user granting a user a subscription upon some event.

Consider one or many of:

1. Extending the pending subscription feature to cover this use case
2. Documenting the limits of this approach, particularly not to build a system around it
3. Removing this feature

Not all ERC-20 tokens return a boolean for their transfer functions. These tokens are not compatible with the Subscriptions contract as currently written, since it requires that they return true.

Consider using Open Zeppelin’s SafeERC20 helper function if support for these tokens is desired.

The _subscribe has a minor reentrancy vulnerability:

• If the user has a sub, then _unsubscribe is called
• This transfers tokens to the user (for a refund), giving an opportunity for reentrancy
• This reentrancy occurs before subscriptions[user] and the new epoch states get updated.

However, this causes the attacker to lose money, as their old subscription data is overwritten with the new, with no chance to retrieve the funds for the old.

This shouldn't cause any problems. However, if extra safety is desired, consider adding OZ's nonReentrant modifier to subscribe, unsubscribe, and fulfil.

Similar to M-2, locked() also may cause cast truncation and yield incorrect values. See line line 332:

uint256 len = uint256(
    SignedMath.max(
        0,
        **int256(Math.min(block.timestamp, _subEnd)) - int64(_subStart)**
    )
);

This function is not utilized internally so no vulnerability is created within this contract.

Consider restricting subscription start and end to a maximum value of type(int64).max.

The values for epochSeconds and uncollectedEpoch may be beneficial for off-chain consumers. Consider adding these to the Init event.

The following comment is inaccurate:

/// @dev Second param required, but currently unused.

Consider updating the comment accordingly.

Calling fulfil() is called after the pending subscription’s end timestamp causes the transaction to revert. No harm is done in this scenario except perhaps to user/developer experience.

Consider adding a validation check against the underflow to provide a useful error message. Note that this is likely uncommon case.

msg.sender can never be zero in unsubscribe(). Consider removing this check.

In fulfil(), minAmount is assigned to the same value as amountUsed. Consider consolidating them into one variable.

Repeated reads and writes to the same storage is discounted, but still expensive. Consider assigning uncollectedEpoch to a new local variable for use within the collect() loop, and then writing its value back to uncollectedEpoch after the loop completes.

This reduces n storage writes to 1 storage write, saving gas.

In unsubscribe():

• The subscription is required to be not expired (sub.end > _now)
• Therefore, the if / else if can be simplified to an if / else
• Then, the setEpochs(sub.start, sub.end, -int128(sub.rate)); can be moved out and above the if / else statements, as it should occur in both cases.

As described in TMAAR, the responsibility of setting the correct subscription rate is solely on the user. When working with this smart contract:

• Users should be wary of mistakes in setting a subscription rate, as an incorrect rate can cause loss of funds to the user.
• Gateways should be wary when upgrading their off-chain rate validation, and consider backwards compatibility for existing subscriptions.