# CryptaSubscription Security Review

A security review of the [CryptaSubscription](https://github.com/0xTC0/crypta-smart-contracts) smart contract protocol was done by [Parth](https://twitter.com/__parthpatel__). \
This audit report includes all the vulnerabilities, issues and code improvements found during the security review.

## Disclaimer

"Audits are a time, resource and expertise bound effort where trained experts evaluate smart
contracts using a combination of automated and manual techniques to find as many vulnerabilities
as possible. Audits can show the presence of vulnerabilities **but not their absence**."

\- Secureum

### Impact

- **High** - leads to a significant material loss of assets in the protocol or significantly harms a group of users.
- **Medium** - only a small amount of funds can be lost (such as leakage of value) or a core functionality of the protocol is affected.
- **Low** - can lead to any kind of unexpected behaviour with some of the protocol's functionalities that's not so critical.

### Likelihood

- **High** - attack path is possible with reasonable assumptions that mimic on-chain conditions and the cost of the attack is relatively low to the amount of funds that can be stolen or lost.
- **Medium** - only conditionally incentivized attack vector, but still relatively likely.
- **Low** - has too many or too unlikely assumptions or requires a huge stake by the attacker with little or no incentive.

### Actions required by severity level

- **Critical** - client **must** fix the issue.
- **High** - client **must** fix the issue.
- **Medium** - client **should** fix the issue.
- **Low** - client **could** fix the issue.

## Executive summary

### Overview

| | |
| :------------ | :------------------------------------------------------------------------------------------- |
| Project Name | Crypta Digital |
| Repository | https://github.com/0xTC0/crypta-smart-contracts |
| Commit hash | [1a5a6c64946d350cbff8e587a7a4eac89e9e6770](https://github.com/0xTC0/crypta-smart-contracts/tree/1a5a6c64946d350cbff8e587a7a4eac89e9e6770) |
| Documentation | [docs](https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/README.md) |
| Methods | Manual review |
| |

### Issues found

| Severity | Count | Resolved |
| :------------ | -------------------------------------------------------- | -------------------------------------------------------- |
| High risk | 7| 7 |
| Medium risk | 2 | 2|
| Low risk | 2 | 2|
| Informational | 4 | 4|

# Findings

## High severity

### [H-1] Owner can overwrite subscription time while creating new subscriptionTimes

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L283-L293
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L170-L171

#### **Description**

When owner or admin tries to add new subscription time, it will overwrite previously created `subscriptionTimes`. This is because `monthly` and `yearly` `subscriptionTimes` are added in the constructor but the value of `_subscriptionTimesNum` is not kept in sync with `subscriptionTimes`.

Status: Resolved

#### **Recommended Mitigation Steps**

Set the value of `_subscriptionTimesNum` in constructor when they are added.

### [H-2] Owner can overwrite subscription type while creating new subscriptionTypes

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L313-L318
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L164-L167

#### **Description**

When owner or admin tries to add new subscription type, it will overwrite previously created `subscriptionTypes`. This is because `light`, `plus`, `pro` and `partner` `subscriptionTypes` are added in the constructor but the value of `_subscriptionTypesNum` is not kept in sync with `subscriptionTypes`.

Status: Resolved

#### **Recommended Mitigation Steps**

Set the value of `_subscriptionTypesNum` in constructor when they are added.

### [H-3] Wrong assignment while creating `subscriptionTypes`

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L167
#### **Description**

In the constructor, while creation of `partner (SubscriptionType(4, "partner"))` subscriptionType, the index assigned to `subscriptionTypes` mapping is `5`. This will lead to failure of checking conditions of comparing `subscriptionTypes[payment.typeId].id` to `payment.typeId` in `giftSubscription` and `_handleSubscription`.

#### **Recommended Mitigation Steps**

Assign the `id` of `4` to index `4` as follows:
`subscriptionTypes[4] = SubscriptionType(4, "partner");`

Status: Resolved

### [H-4] Modification of `CryptaKeys` contract address will make previous NFT owners to lose power

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L205-L207
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L158-L161
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L509-L517
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L501-L507

#### **Description**

Function `setKeysAddress` is owner controlled which modifies the `keys` address. The holder of the keys have some powers like withdrawing the funds to `vaultWallet`. Thus, if the `keys` address is modified by the owner, all the previous NFT holders will lose their power.

#### **Recommended Mitigation Steps**

No specific recommendation for this as it is choice by design.

Status: Resolved

### [H-5] Refunding for specific `_subId` refunds full amount

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L350-L374

#### **Description**

`refund` is an `owner` controlled function which tries to refund if someone cancels the `_subId`. But it returns the full amount instead of refunding on pro-rated basis. The users can exploit this by claiming refund at the end.

#### **Recommended Mitigation Steps**

Implement functionality to refund prorated amount instead of full amount.

Status: Resolved

### [H-6] Anyone can call `executePayment` on `_subId` even if subscription is cancelled for ERC20 based subscription.

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L380-L399
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L350-L374

#### **Description**

`executePayment` can be called by anyone can it is used for renewal of expired subscription. But for cancelled subscription, it is possible to execute the payment and pull erc20 token from the customer.

#### **Recommended Mitigation Steps**

Put a condition to stop users from calling `executePayment` if subscrition is refunded.

Status: Resolved

### [H-7] `_comissionPayoutErc20` always fails due to use of `safeTransferFrom`

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L577-L586
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L429-L450
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L477-L499

#### **Description**

`_comissionPayoutErc20` uses `safeTransferFrom` to transfer Erc20 token from contract to affiliate. This transaction will always fail because contract won't have any allowance for itself.

#### **Recommended Mitigation Steps**

Use `safeTransfer` instead of `safeTransferFrom`.

Status: Resolved

## Medium severity

### [M-1] `extendSubscription` doesn't check if the `_timestamp` for `_sudId` is more than previous `endTime`

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L270-L276

#### **Description**

`extendSubscription` is a function which modifies `endTime` for the subscription. However, it doesn't perform sanity checks on the input value and it is possible to set `endTime` less than previous instead of extending it.

#### **Recommended Mitigation Steps**

Perform sanity checks on input value.

Status: Resolved

### [M-2] Wrong require check on `executePayment`.

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L383

#### **Description**

`subscriptions[_subId].endTime` is the time at which the subscription is expired and it should be possible to `executePayment` at the timestamp equal to `endTime`. So, `require` check should be modified.

#### **Recommended Mitigation Steps**

Change the line as following:
`require(subscriptions[_subId].endTime <= block.timestamp, "SUB_ACTIVE");`

Status: Resolved

## Low severity

### [L-1] missing modifier in `setKeysAddress` function.

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L205-L207

#### **Description**

`validAddress` modifier is missing in `setKeysAddress`. So, it is possible to set address(0) for keys.

#### **Recommendations**

Use `validAddress` modifier for `setKeysAddress` function.

Status: Resolved

### [L-2] No upper bound on `affiliateFee`.

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L429
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L477

#### **Description**

There is no upper bound on what values can be set as an `affiliateFee`.

#### **Recommendations**

Use upperbound for `affiliateFee`.

Status: Resolved

### [L-2] `affiliateFee` can be `0` while `affiliateAddress` can be non-zero while creation of subscription

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L418
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L465

#### **Description**

There can be issue when `affiliateFee` is `0` and `affiliateAddress` is non-zero. The function used for creating subscriptions only checks for `affiliateFee`.

Status: Resolved

#### **Recommendations**

The function used for creating subscriptions should also checks for `affiliateAddress` to be zero when they expect `affiliateFee` to be zero.

## Informational

### [I-1] Unused import

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L4

#### **Description**

`SafeMath` library is imported which is not used anywhere in the contract.

#### **Recommendations**

Remove unused import.

Status: Resolved

### [I-2] The first require condition in `giftSubscription` is already checked in underlying function call.

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L257
- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L524

#### **Description**

`giftSubscription` checks following condition:

`require(subscriptionTypes[payment.typeId].id == payment.typeId,"INVALID_SUBTYPE");`

The above condition is also checked in `_handleSubscription` function which is called inside `giftSubscription` making the condition useless.

#### **Recommendations**

Remove the mentioned require condition from `giftSubscription`.

Status: Resolved

### [I-3] Typo in comment of `executePayment` function

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L378

#### **Description**

The function information comment of `executePayment` function has small typo and doesn't makes sense. In the code, it is: `@param _subId and id of active subscription`

#### **Recommendations**

Change this to: `@param _subId an id of active subscription`

Status: Resolved

### [I-4] Unused function

#### **Context**

- https://github.com/0xTC0/crypta-smart-contracts/blob/1a5a6c64946d350cbff8e587a7a4eac89e9e6770/CryptaSubscription.sol#L592-L596

#### **Description**

Function `eq` is not used anywhere in the contract.

#### **Recommendations**

Try removing unused function to save deployed contract bytecode.

Status: Resolved