# V12 Vault Manager Audit by CryptoManiacs

### Authors:

- Anton Bukov
- Mikhail Melnik

### Scope:

- https://github.com/vault12/vault_manager/tree/d4491b0af204554a064cabeed1a853f15b0e307c

### Summary:

Provided smart contracts present system for charging annual payments in ETH and ERC20 currencies potentially from infinite number of users without enumerating them on any of the steps. Tarrifs duration restricted to be the same for every user to achieve convinient way to terminate expired subscriptions one by one.

CryptoManiacs team performed security audit of V12 Vault Manager smart contract including automatic and manual source code analysis. The issues we found were classified by severity (see Appendix 1) and included in the following report. Developer fixed critical, medium and some minor issues, and was informed about other minor issues.

CryptoManiacs team consider V12 Vault Manager smart contracts as safe to use in Ethereum Mainnet network.

## Issues:

### 1. Wrong calculation of return in `_extendSubscription`

- *Severity*: Critical

- *Location*: [`SubscriptionETH.sol#L55`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionETH.sol#L55)

    uint change = payment.sub(msg.value);

- *Comment*: `payment` is almost always less than `msg.value` so this statement will almost always revert txn. Should be `uint change = msg.value.sub(payment);`

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#df67ac5f6fd171e9e5fb782f65697f0b3ee5db1d](https://github.com/vault12/vault_manager/pull/2/commits/df67ac5f6fd171e9e5fb782f65697f0b3ee5db1d)
- *Auditors*: Verified

### 2. Extension subsription payment calculation is wrong

- *Severity*: ~~Critical~~ Not an issue

- *Location*: [`BaseSubscription.sol#L266`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L266)

- *Comment*: Extension payment logic seems to be broken. It works only in case if extended price is the same as the old price. `newDepositValue` should be equal to `_price`. `payment` should be equal to `_price - subscription.depositValue + paidEther`. In case of extended price is lower than the old price it might be the case when leftover of user's funds is more than enough to pay for the full extension and in that case the refund of extra funds is required in `extendSubscription`.

- *Status*:

- *Developers*: Works as designed. Have a look at the tests and documentation. [#000f0acf28241b6530c1f67e3a4efe716607b621](https://github.com/vault12/vault_manager/pull/2/commits/000f0acf28241b6530c1f67e3a4efe716607b621). For the case you are describing users can always do a combination of `unsubscribe` and `subscribe` calls.
- *Auditors*: Verified, changed severity to *Not an issue*.

### 3. Case when withdrawals gets locked

- *Severity*: Medium

- *Location*: [`BaseSubscription.sol#L312`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L312)

    uint paidTime = billing.previousClearingDate.sub(subscription.startTimestamp);
    uint etherToCharge = subscription.depositValue.sub(chargedEther);

- *Comment*: Following lines could revert in case `withdrawal` method will not be called during `duration`. This seems very low-chance to happend, but theoretically is possible. Use this pseudocode:

    uint paidTime = Math.max(0, int(billing.previousClearingDate) - int(subscription.startTimestamp));

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors.. [#a150b9b050cc75325971ab04e3bf42b407ff7aaf](https://github.com/vault12/vault_manager/pull/2/commits/a150b9b050cc75325971ab04e3bf42b407ff7aaf)
- *Auditors*: Verified

### 4. Why to have `canceled` state of subscription?

- *Severity*: Medium

- *Location*:

- [`BaseSubscription.sol`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol)

- *Comment*: It seems canceled state can be removed using `_clearSubscription` without losing any logic. Just erase `subscriptions` and `orderedSubscription` records when user cancel subcription. This would reduce number of states and allows to simplify logic in some places. We mean cancelled subsription should behave as [disabled one](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L322).

- *Status*:

- *Developers*: We want to keep it as is to preserve the history for convineince
- *Auditors*: History usually can be retrieved from events/log, but if you wish to keep - ok

### 5. `SignatureLib` duplicates 2 existing OZ library methods

- *Severity*: Low

- *Location*: [`SignatureLib.sol`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SignatureLib.sol)

- *Comment*: Most of the code duplicates existing OZ functionality, please utilize it without code duplication: [https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol). You may also like an idea to get rid of `SignatureLib`, because `enum Plan` seems more related to business logic that to reusable library.

- *Status*:
- *Developers*: Fixed by dev team. Awaiting verification by auditors.. [#7958af7cc4306711273b042f65fc3de10eb3826f](https://github.com/vault12/vault_manager/pull/2/commits/7958af7cc4306711273b042f65fc3de10eb3826f)

- *Auditors*: Verified

### 6. Methods for mandatory overriding should have no implementation

- *Severity*: Low

- *Location*:

- [`BaseSubscription.sol#L92`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L92)
- [`BaseSubscription.sol#L203`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L203)
- [`BaseSubscription.sol#L301`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L301)
- [`BaseSubscription.sol#L363`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L363)
- [`BaseSubscription.sol#L366`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L366)

- *Comment*: Just remove `{}` to achieve abstract contracts and instantiation will be denied until method inplemented in subcontract.

    function _upgradePlan(uint _price) internal {}

=>

    function _upgradePlan(uint _price) internal;

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors.. [#842cc20ecf4b3cbbe82599135711df354e47907e](https://github.com/vault12/vault_manager/pull/2/commits/842cc20ecf4b3cbbe82599135711df354e47907e). Note: we decided to leave the `_payToOwner` and `_payToUser` funcs as are, because it's required to many changes to accomplish the goal ("Functions without implementation cannot have modifiers").
- *Auditors*: This exact method implementation is never called, so modifiers will never work. You can achieve this by the following code:

        contract Base {
            function _payToOwner(uint value) private nonReentrant {
                _payToOwnerInternal(value);
            }
            
            function _payToOwnerInternal(uint value) internal;
        }
        
        contract Concrete {
            function _payToOwnerInternal(uint value) internal {
                owner().transfer(value);
            }
        }

- *Developers:* final fix [#585a6476465ea928a3cc22a7e9daa9a859ff9afe](https://github.com/vault12/vault_manager/pull/2/commits/585a6476465ea928a3cc22a7e9daa9a859ff9afe)

- *Auditors*: Verified

### 7. Method `subscribe` could be `external` instead of `public` as many others

- *Severity*: Low

- *Location*: [`BaseSubscription.sol#L59`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L59)

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#886a81985c22e7d9664741904944c912671612de](https://github.com/vault12/vault_manager/pull/2/commits/886a81985c22e7d9664741904944c912671612de)
- *Auditors*: Verified

### 8. Why to subclass library instead of just using it?

- *Severity*: Low

- *Location*: [`BaseSubscription.sol#L9`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L9)

- *Comment*: Libraries can be used inline without separate deployment in case of `internal` methods modifiers.

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#7958af7cc4306711273b042f65fc3de10eb3826f](https://github.com/vault12/vault_manager/pull/2/commits/7958af7cc4306711273b042f65fc3de10eb3826f)
- *Auditors*: Verified

### 9. Suggestion to use new `SafeMath.sub(a,b,strErr)`

- *Severity*: Low

- *Location*: [`SubscriptionETH.sol#L22`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionETH.sol#L22)

    require(msg.value >= payment, "not enough ETH");
    uint change = msg.value.sub(payment);

=>

    uint change = msg.value.sub(payment, "not enough ETH");

- *Comment*: Latest `@openzeppelin/contracts` 2.4.0 provides convinient `sub` method with additional argument for pointing error. Let's use latest version of library with this feature for some substractions? https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/math/SafeMath.sol#L57

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#17504e2e3d78b6208ff8826c6df80717e235f7e6](https://github.com/vault12/vault_manager/pull/2/commits/17504e2e3d78b6208ff8826c6df80717e235f7e6)
- *Auditors*: Verified

### 10. Deny `ticketProvider` to be `0x0`

- *Severity*: Low

- *Location*: [`BaseSubscription.sol#L360`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol#L360)

- *Comment*: Let's check that `ticketProvider` is not `0x0` in initializer, else any invalid signature will be recognized as valid.

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#17504e2e3d78b6208ff8826c6df80717e235f7e6](https://github.com/vault12/vault_manager/pull/2/commits/17504e2e3d78b6208ff8826c6df80717e235f7e6)
- *Auditors*: Verified

### 11. Rename `isLastSubscriptionExpired` to `isOldestSubscriptionExpired` to conform `disableOldestSubsription` naming.

- *Severity*: Low

- *Location*: [`BaseSubscription.sol`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol)

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#ae17694ebe1d802a51eb63e8c7b013ac8ef8111e](https://github.com/vault12/vault_manager/pull/2/commits/ae17694ebe1d802a51eb63e8c7b013ac8ef8111e)
- *Auditors*: Verified

### 12. Methods `upgradeSubscription` and `downgradeSubscription` have duplicating code

- *Severity*: Low

- *Location*: [`BaseSubscription.sol`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/BaseSubscription.sol)

- *Comment*: Duplicating code could be extracted to something like `_changeSubscription`.

- *Status*:

- *Developers*: Please provide an example. We don't think there is enough code to abstract
- *Auditors*: Agreed to keep current version

### 13. Logic duplication across ETH and token implementation

- *Severity*: Low

- *Comment*: Consider using `UniversalERC20`-like implementation to have ETH+ERC20 abstration: [https://gist.github.com/k06a/201149189b259277cf3e42b7727eafa7](https://gist.github.com/k06a/201149189b259277cf3e42b7727eafa7). For example you could use:

    require(msg.value >= payment, "not enough ETH");
    uint change = msg.value.sub(payment);
    if (change > 0) {
        _payToUser(change);
    }

Or

    token.safeTransferFrom(msg.sender, address(this), payment);

=>

    token.universalTransferFrom(msg.sender, address(this), payment);

- *Status*:

- *Developers*: We don't believe it's safe to use such code especially with `eth.transfer` calls due to Instanbul hf. We also think it reduces readability of current code and we know that it will work for VGT token. We believe, we should leave as is, if it's safe
- *Auditors*: Agreed to keep current version

### 14. Most `transferFrom` call in requires will not work as expected

- *Severity*: Low

- *Location*:

- [`SubscriptionVGT.sol#L22`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionVGT.sol#L22)

- [`SubscriptionVGT.sol#L34`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionVGT.sol#L34)

- [`SubscriptionVGT.sol#L48`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionVGT.sol#L48)

- *Comment*: Token will revert in case of not enough VGT token were approved or low balance. So this revert reason is not sufficient. I'd suggets to use `SafeERC20` or even `UniversalERC20` if you like.

    require(
        token.transferFrom(msg.sender, address(this), payment),
        "Not enough VGT"
    );

=>

    token.safeTransferFrom(msg.sender, address(this), payment);

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#32e914d94ff8ffeed367888dee943d898b7f38fb](https://github.com/vault12/vault_manager/pull/2/commits/32e914d94ff8ffeed367888dee943d898b7f38fb)
- *Auditors*: Verified

### 15. Why to use old-school gas-eating asserts instead of reverts

- *Severity*: Low

- *Location*:

- [`SubscriptionVGT.sol#L40`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionVGT.sol#L40)

- [`SubscriptionETH.sol#L31`](https://github.com/vault12/vault_manager/blob/d4491b0af204554a064cabeed1a853f15b0e307c/contracts/SubscriptionETH.sol#L31)

    if (payment > 0) {
        ...
    } else if (refund > 0) {
        ...
    } else {
        assert(refund == 0 && payment == 0);
        ...
    }

- *Comment*: Also it is not clear what exactly this code checks, because it seems it is not possible to reach it with different condition.

- *Status*:

- *Developers*: Fixed by dev team. Awaiting verification by auditors. [#257026508eed69bb55da6e37e2e1d3764a8347bc](https://github.com/vault12/vault_manager/pull/2/commits/257026508eed69bb55da6e37e2e1d3764a8347bc)
- *Auditors*: Verified

# Appendix 1 - Terminology

## Severity

Assessment of the magnitude of an issue.

||
|-----:|
|Picture 1. Severity|

### Minor (Low)

Minor issues are generally subjective in nature or potentially associated with the topics like “best practices” or “readability”. As a rule, minor issues do not indicate an actual problem or bug in the code.

The maintainers should use their own judgment as to whether addressing these issues will improve the codebase.

### Medium

Medium issues are generally objective in nature but do not represent any actual bugs or security problems.

These issues should be addressed unless there is a clear reason not to.

### Major (High)

Major issues are things like bugs or vulnerabilities. These issues may be unexploitable directly or may require a certain condition to arise in order to be exploited.

If unaddressed, these issues are likely to cause problems with the operation of the contract or lead to situations which make the system exploitable.

### Critical

Critical issues are directly exploitable bugs or security vulnerabilities.

If unaddressed, these issues are likely or guaranteed to cause major problems or ultimately a full failure in the operations of the contract.