Subscription Token Protocol V2

In essence, the calculation of points is incorrect during slashing as it does not take into account the variable `totalRewardsEgress` (i.e. the function `RewardsPoolLib.burn()`). This can cause two undesired outcomes when a user tries to claim their rewards: 1. He may be denied claiming the rewards as there are not enough funds left in the contract. 1. If there are enough funds left in the contract, he may be able to claim more rewards than he is entitled to. <br> To illustrate the issue, consider the following example: ``` Assume `Expiry Time = 1 block` and no slashing grace period. Block 1: (Multiplier = 2) Bob purchases 50 tokens => 100 shares cuz of 2x multiplier Total Share = 100 pointsPerShare = 50/100 = 0.5 totalRewardIngress = 50 totalRewardEgress = 0 Bob has 100 shares, and Bob's pointsCorrection = 0 Alice has 0 shares, and Alice's pointsCorrection = 0 Block 2: (Multiplier decreases to 1) Alice purchases 50 tokens => 50 shares cuz of 1x multiplier Total Share = 150 pointsPerShare += 50/150 = 0.8333... totalRewardIngress = 100 totalRewardEgress = 0 Bob has 100 shares, Bob's pointsCorrection = 0, and Bob rewardsWithdrawn = 0 Alice has 50 shares, Alice's pointsCorrection = -0.5*50 = -25, and Alice rewardsWithdrawn = 0 Block 3: Alice claims her rewards with `STPV2.transferRewardsFor(Alice)`. Alice claims (0.83 * 50) - 25 = 16.66666.. tokens as rewards. Total Share = 150 pointsPerShare += 50/150 = 0.8333... totalRewardIngress = 100 totalRewardEgress = 16.66666... Bob has 100 shares, Bob pointsCorrection = 0, and Bob rewardsWithdrawn = 0 Alice has 50 shares, Alice pointsCorrection = -0.5*50 = -25, and Alice rewardsWithdrawn = 16.6666666 Note: Bob (can) claim 83.333333 here, and can possibly be slashed. But suppose these do not happen here. Block 4: Alice is slashed, but Bob is not slashed. Total Share = 100 pointsPerShare = 100/100 = 1 totalRewardIngress = 100 totalRewardEgress = 16.66666... Bob has 100 shares, Bob pointsCorrection = 0, and Bob rewardsWithdrawn = 0 Alice has 0 shares, Alice pointsCorrection = 0, and Alice rewardsWithdrawn = 0 Block 5: Bob claims his rewards with `STPV2.transferRewardsFor(Bob)`. Bob claims (1 * 100) - 0 = 100 tokens as rewards. Total Share = 100 pointsPerShare = 1 totalRewardIngress = 100 totalRewardEgress = 116.66666... Bob has 100 shares, Bob pointsCorrection = 0, and Bob rewardsWithdrawn = 100 Alice has 0 shares, Alice pointsCorrection = 0, and Alice rewardsWithdrawn = 0 However, this will revert as there are only 83.33 tokens in the contract. ``` This is especially problematic because anyone can call slashing on behalf of other users, greatly amplifying the risks of the above situation happening.

This vulnerability was brought to our attention by the Subscription Token Team. `STPV2.mintAdvanced()` allows callers to specify both the recipient and the tier when purchasing a subscription. This creates a situation in which users can purchases subscriptions for another user with the intent of blocking them out of a favorable subscription tier. This becomes problematic in the situation where there is a highly exclusive subscription tier. The potential reward benefits from joining that tier could be more profitable than it would be to purchase subscriptions of a lower tier for an address that one may suspect to try to join the higher tier.

While reentrancies may not always negatively impact a system, their existence should be highlighted and their potential consequences mitigated. Here are the instances identified: 1. via `_checkOnERC721Received()`: - In `STPV2._purchase()`, a token can be minted via `_safeMint()`. It can trigger the execution of `_checkOnERC721Received()` that would be able to interact with the system in a read or write access in a state that may be inconsistent since any storage variable that remains to be updated after the call to `_safeMint()` are still in their initial state. - Same remark for `STPV2.grantTime()`. 2. via `CurrencyLib.capture()`, a reentrancy could happen in `STPV2.yieldRewards()` and `STPV2._purchase()`. 3. via `CurrencyLib.transfer()`, a reentrancy could happen in `SPTV2Factory.deploySubscription()`, `SPTV2._transferFee()` and `SPTV2._purchase()`. 4. via `GateLib.balance()`, a reentrancy could happen in `TierLib.checkJoin()`.

After a certain time (i.e. `periods > curve.numPeriods`), the curve multiplier will be lower bounded by `minMultiplier`, as indicated by `RewardCurveLib.currentMultiplier()`. However, it is possible that for periods before `curve.numPeriods`, the multiplier calculated using `uint256(curve.formulaBase) ** (curve.numPeriods - periods)` is lower than the `minMultiplier`. This means for some misconfigured curves as described above, there may be undesired cases where a user is incentivized to delay purchasing the subscription due to an expected increase in `currentMultiplier()` in later periods (i.e. when `periods > curve.numPeriods`).

At several locations, values are downcasted. Some of them may have an initial value higher than the max value supported by the destination type and this could lead to unsafe consequences instead of reverting. It is the case in the contracts: * `SubscriberLib.sol`; * `GateLib.sol`; * `TierLib.sol`; * `SubscriptionLib.sol`; * `RewardPoolLib.sol`;

To validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transitions can be logged, which is beneficial for monitoring the contract and tracking eventual bugs or hacks. Below is a list of events that could be improved: 1. In `STPV2.updateProtocolFeeRecipient()` and `STPV2.updateClientFeeRecipient()`, the new address is not logged and the same event `FeeRecipientChange` is emitted in both functions. 2. In `STPV2Factory.setDeployFee()`, no event is emitted.

`ERC4906` expects the event `MetadataUpdate` to be emitted when an update happens. However, in the functions `SubscriberLib.extend()` and `SubscriberLib.retract()`, these events may be emitted even if the new value is the same as the old one. `ERC5192` expects events `Locked` and `Unlocked` to be emitted when the locked status of an object is updated. However: * in the function `emitSwitchEvents()`, it is possible to emit the same event in a row depending on the value of `transferrable`. * when a tier is updated and its attribute `transferrable` is updated, no event is emitted for existing tokens. * when tokens are minted for a given tier, no `ERC5192` event is emitted. This could impact the operations of external observers of what happens on-chain.

The function `STPV2Factory.deploySubscription()` emits `Deployment` events with the field `bytes deployKey` when a user deploys a subscription. According to `Factory.sol`, this field is "an identifier to help track deployments via Deploy event". However, since this value is provided by the user deploying the contract and in the absence of a unicity check, anyone can front-run or back-run a subscription deployment with the same `deployKey` value. As a result, external observers could be misled and subscribe to the contract deployed by the impersonator. This risk should be documented to creators and subscribers.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error: In `STPV2.sol`: - qs-badge-acknowledged `setGlobalSupplyCap()`, `supplyCap` can be set to 0; - qs-badge-acknowledged Any address can be used as the `_currency`. Advise users to check the address used as the currency of a given subscription before performing transactions with this contract. - qs-badge-fixed *NOTE: This missing input validation was brought to our attention by the Subscription Token Team.* `STPV2.initialize()` does not ensure that the specified client or protocol recipient are different from `address(0)` in the case where the fees defined are greater than zero. If a subscription can be created with a non-zero fee and a fee recipient of the zero address, then all mints of that subscription will fail, as the `CurrencyLib` prevents transfers to the zero address. - qs-badge-fixed `issueRewardShares()` does not ensure that the `account` is different from `address(0)`, meaning rewards could be issued to the zero address <br> In `ReferralLib.sol`: - qs-badge-fixed `setReferral()`, `settings.basisPoints` can have a value of maximum `10000`. <br> In `SubscriptionLib.sol` - qs-badge-fixed *NOTE: This missing input validation was brought to our attention by the Subscription Token Team.* In-line comments in the struct definition for `Tier` in `Index.sol` describe that a `maxSupply` of zero indicates no maximum value which supports an unlimited number of subscribers. However, In `updateTier()` if there are active subscriptions on the tier, and the `maxSupply` is being updated to zero, the update will revert as the `subCount` exceeds zero. Consider using `type(uint256).max` to indicate an unlimited supply, or, check against `maxSupply` equaling zero. <br> In `TierLib.sol`: - qs-badge-acknowledged In `validate()`: - the `periodDurationSeconds` can be very small; - `rewardCurveId` is not checked to match an existing curve; - the immutability of some parameters could lead to abuses by the creator. For instance, `transferrable` and `maxSupply` can be updated once people already bought a subscription, which could affect the value of that subscription. <br> In `Rewards.sol`: - qs-badge-acknowledged The fact that rewards can be slashable with a `slashGracePeriod` of `0` should be confirmed to be the documented behavior. The same recommendation is for the mutability nature of these two parameters.

The function `GateLib.validate()` reverts if the `gate` used an address of type `ERC-1155` and `tokenId == 0`. However, some ERC-1155 may have a valid `tokenId == 0`. This should be documented to creators.