Subscription Token Protocol V2 security record

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.

A security issue has been identified within the reward redemption and total reward points management system of the platform. This issue is critical because it can result in unfair reward distribution, where users who have accumulated reward points may not receive the correct amount of tokens from the reward pool. The issue arises from the formula used for redemption and the dynamics of total reward points. Analysis: 1. Reward Redemption Formula: - The platform uses a redemption formula to calculate the number of tokens a user receives from the reward pool when redeeming their reward points. - The formula is: `rewardPoolTotal * (userRewardPoints / totalRewardPoints)`. - `rewardPoolTotal` represents the total amount of tokens allocated for rewards (not the current reward pool balance). - `totalRewardPoints` is the sum of reward points in circulation. 2. Total Reward Points Dynamics: - `totalRewardPoints` increases when users purchase subscription time. - `totalRewardPoints` decreases when a user's subscription expires, and their reward points are slashed by other users. - The formula for reward points redemption can potentially fail to accurately distribute rewards when `totalRewardPoints` decreases, leading to funds blocked in the contract. When a user subscription expires, other users can slash and burn those reward points, so `totalRewardPoints` will decrease and users will get a higher share per reward token on the reward pool. `rewardPoolBalance` will not have enough funds for the users, and some withdrawals will revert, locking the funds in the contract.

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.

The [Checks-Effects-Interactions](https://solidity.readthedocs.io/en/latest/security-considerations.html#use-the-checks-effects-interactions-pattern) coding pattern is meant to mitigate any chance of other contracts manipulating the state of the blockchain in unexpected and possibly malicious ways before control is returned to the original contract. As the name implied, only after checking whether appropriate conditions are met and acting internally on those conditions should any external calls to, or interactions with, other contracts be done. The following instance(s) have been identified: <br> * `_refund()` <br> ``` uint256 tokens = balance * _tokensPerSecond; if (balance > 0) { sub.secondsPurchased -= balance; _transferOut(account, tokens); <== External Call } _subscriptions[account] = sub; <== State Change ``` <br> * `_fetchSubscription()` <br> ``` if (sub.tokenId == 0) { require(_supplyCap == 0 || _tokenCounter < _supplyCap, "Supply cap reached"); _tokenCounter += 1; sub = Subscription(_tokenCounter, 0, 0, block.timestamp, block.timestamp, 0, 0, 0); <== This state gets updated in `purchaseTime()` _safeMint(account, sub.tokenId); <== External Call } ```

A scenario where a content creator has lost access to their private keys, and did not set a designated transfer recipient (`_transferRecipient`) will lead to funds locked in the contract. This oversight results in rewards (if activated) and fees not being correctly allocated, causing these funds to remain locked within the contract indefinitely. Rewards and fees are allocated when `withdrawTo()` is executed. Note that this function can only be called by the contract creator. Users will not be able to claim any reward tokens by redeeming their reward points. Although this can be seen as a contract creator's responsibility, final user funds will be locked, potentially harming the protocol's reputation.

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.