fix: when extending make sure user does not pay for time in the past …

…(0xM H-01) (#86)

* fix: when extending make sure user does not pay for time in the past (0xM H-01)

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

* fix: revert when extending if amount is not multiple of rate (0xM L-02) (#87)

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

contracts/contracts/Subscriptions.sol

@@ -268,7 +268,7 @@ contract Subscriptions is Ownable {
     /// @notice Extends a subscription's end time.
     /// The time the subscription will be extended by is calculated as `amount / rate`, where
     /// `rate` is the existing subscription rate and `amount` is the new amount of tokens provided.
-    /// The new end time must be in the future.
+    /// If the subscription was expired the extension will start from the current block timestamp.
     /// @dev The function's name, `addTo`, is used to comply with the `IPayment` interface for recurring payments.
     /// @param user Subscription owner.
     /// @param amount Total amount to be added to the subscription.
@@ -279,10 +279,10 @@ contract Subscriptions is Ownable {
         Subscription memory sub = subscriptions[user];
         require(sub.start != 0, 'no subscription found');
         require(sub.rate != 0, 'cannot extend a zero rate subscription');
+        require(amount % sub.rate == 0, "amount not multiple of rate");
 
-        uint64 oldEnd = sub.end;
-        uint64 newEnd = oldEnd + uint64(amount / sub.rate);
-        require(block.timestamp < newEnd, 'new end cannot be in the past');
+        uint64 newEnd = uint64(Math.max(sub.end, block.timestamp)) +
+            uint64(amount / sub.rate);
 
         _setEpochs(sub.start, sub.end, -int128(sub.rate));
         _setEpochs(sub.start, newEnd, int128(sub.rate));
@@ -292,7 +292,7 @@ contract Subscriptions is Ownable {
         bool success = token.transferFrom(msg.sender, address(this), amount);
         require(success, 'IERC20 token transfer failed');
 
-        emit Extend(user, oldEnd, newEnd, amount);
+        emit Extend(user, sub.end, newEnd, amount);
     }
 
     function setRecurringPayments(address _recurringPayments) public onlyOwner {

contracts/test/contract.test.ts

@@ -3,7 +3,7 @@ import {time} from '@nomicfoundation/hardhat-network-helpers';
 
 import {expect} from 'chai';
 import * as deployment from '../utils/deploy';
-import {getAccounts, Account, toGRT} from '../utils/helpers';
+import {getAccounts, Account, toGRT, provider} from '../utils/helpers';
 
 import {Subscriptions} from '../types/contracts/Subscriptions';
 import {StableToken} from '../types/contracts/test/StableMock.sol/StableToken';
@@ -624,7 +624,7 @@ describe('Subscriptions contract', () => {
       await expect(tx).revertedWith('no subscription found');
     });
 
-    it('should revert when the new end time is in the past', async function () {
+    it('should allow extending an expired subscription', async function () {
       const now = await latestBlockTimestamp();
       const start = now.add(500);
       const end = now.add(1000);
@@ -643,24 +643,37 @@ describe('Subscriptions contract', () => {
       // mine past the newEnd
       await mineNBlocks(1500);
 
-      const tx = addToSubscription(
+      await addToSubscription(
         stableToken,
         subscriptions,
         recurringPayments,
         subscriber1.address,
         amountToExtend,
         subscribeBlockNumber
       );
-      await expect(tx).revertedWith('new end cannot be in the past');
     });
-
+
+    it('should not allow extending an active subscription if amount is not multiple of rate', async function () {
+      const now = await latestBlockTimestamp();
+      const start = now;
+      const end = now.add(1000);
+      const rate = BigNumber.from(7);
+      const amountToExtend = BigNumber.from(2000); // newEnd: end + 2000/7 = 1000 + 286 = 1286
+
+      // mine past the start of the subscription
+      await mineNBlocks(150);
+
+      const tx = subscriptions.addTo(subscriber1.address, amountToExtend);
+      await expect(tx).revertedWith('amount not multiple of rate');
+    });
+
     it('should allow extending an active subscription', async function () {
       const now = await latestBlockTimestamp();
       const start = now;
       const end = now.add(1000);
       const rate = BigNumber.from(5);
       const amountToExtend = BigNumber.from(2000); // newEnd: end + 2000/5 = 1000 + 400 = 1400
-      
+
       const subscribeBlockNumber = await subscribe(
         stableToken,
         subscriptions,
@@ -689,7 +702,7 @@ describe('Subscriptions contract', () => {
       const end = now.add(1000);
       const rate = BigNumber.from(5);
       const amountToExtend = BigNumber.from(2000); // newEnd: end + 2000/5 = 1000 + 400 = 1400
-      
+
       const subscribeBlockNumber = await subscribe(
         stableToken,
         subscriptions,
@@ -1406,13 +1419,18 @@ async function addToSubscription(
   const beforeContractBalance = await stableToken.balanceOf(
     subscriptions.address
   );
-  const newEnd = beforeSub.end.add(amount.div(beforeSub.rate));
-  // const additionalTokens = beforeSub.rate.mul(newEnd.sub(beforeSub.end));
 
   // * Tx
   const tx = subscriptions.connect(signer.signer).addTo(user, amount);
+  const receipt = await (await tx).wait();
+  const txTimestamp = (
+    await subscriptions.provider.getBlock(receipt.blockNumber!)
+  ).timestamp;
 
   // * Check events
+  const newEnd = BigNumber.from(
+    Math.max(beforeSub.end.toNumber(), txTimestamp)
+  ).add(Math.ceil(amount.toNumber() / beforeSub.rate.toNumber()));
   await expect(tx)
     .to.emit(subscriptions, 'Extend')
     .withArgs(user, beforeSub.end, newEnd, amount);
@@ -1423,9 +1441,7 @@ async function addToSubscription(
     subscriptions.address
   );
   expect(afterBalance).to.eq(beforeBalance.sub(amount));
-  expect(afterContractBalance).to.eq(
-    beforeContractBalance.add(amount)
-  );
+  expect(afterContractBalance).to.eq(beforeContractBalance.add(amount));
 
   // * Check state
   const afterSub = await subscriptions.subscriptions(user);