# Snowswap Security Review

This security assessment was prepared by Quantstamp, the leader in blockchain security

**Date:** 23 November 2020
**Auditors:**
* Sebastian Banescu, Senior Research Engineer
* Kacper Bak, Senior Research Engineer
* Poming Lee, Research Engineer

## Executive Summary

Quantstamp has performed a security review on the diff between the code forked by Snowswap from Curvefi and Uniswap [https://github.com/Uniswap/liquidity-staker/commit/3edce550aeeb7b0c17a10701ff4484d6967e345f](https://github.com/Uniswap/liquidity-staker/commit/3edce550aeeb7b0c17a10701ff4484d6967e345f).

The Snowswap project has forked the following commit hash of Curvefi [https://github.com/curvefi/curve-contract/commit/4bc97e62f380a34900651c3ecc88dd4193a7a40d](https://github.com/curvefi/curve-contract/commit/4bc97e62f380a34900651c3ecc88dd4193a7a40d) which has NOT been audited:
1. The [diff](https://github.com/curvefi/curve-contract/compare/65365742b4e3b644d61443de6feee49af0d2b095...4bc97e62f380a34900651c3ecc88dd4193a7a40d) between this commit and the previously audited commit (by ToB) in March 2020 consists of 29 commits, 27 files changed with 3,194 additions and 311 deletions.
2. The [diff](https://github.com/curvefi/curve-contract/compare/4bc97e62f380a34900651c3ecc88dd4193a7a40d...590550117bfcbd6d1823de732f70b9a03b576cb8) between this commit and the following audited commit (by Quantstamp) in October/November 2020 consists of 318 commit, 216 files changed with 35,034 additions and 7,131 deletions.

There have been numerous bug fixes and changes to the code since between the forked commit and the commits that have been audited. Therefore, we strongly believe that a full code audit would be required to uncover more issues.

During this security review, Quantstamp has identified 10 security issues:
* 4 High severity
* 2 Medium severity
* 2 Low severity
* 2 Informational severity

Additionally, 7 code documentation and best practice issues were identified. The project has not provided us with any specification for the changes made from the aforementioned code forks and has not adjusted the test suites to check the newly added and modified code functions properly.

We recommend properly testing the code and writing a technical specification for the newly added code to complement the existing specification for the forked code.

## Findings

### QSP-1 Geyser contracts do not compile correctly

Severity: High
Affected files: snowswap_staking_geyser_1.sol, geyser_factory.sol

**Description:** The `snowswap_staking_geyser_1.sol` file gives the following compilation errors:
1. DeclarationError: Undeclared identifier. Did you mean "_balances" or "balanceOf"? `if (reward > balance) {`
2. DeclarationError: Undeclared identifier. Did you mean "_balances" or "balanceOf"? `reward = balance;`

The `geyser_factory.sol` file gives the following compilation error: "DeclarationError: Identifier not found or not unique. `IOriginUSD public ousd;`" which indicates that there is no definition for the `IOriginUSD` interface/type.

**Recommendations:** Fix compilation errors by defining/declaring the necessary types.

### QSP-2 Contracts forked from Curvefi are vulnerable to loss-making updates to A

Severity: High
Affected files: curve-contract-pool_yvault/vyper/stableswap.vy

**Description:** The Snowswap project has forked the following commit hash of Curvefi [https://github.com/curvefi/curve-contract/commit/4bc97e62f380a34900651c3ecc88dd4193a7a40d](https://github.com/curvefi/curve-contract/commit/4bc97e62f380a34900651c3ecc88dd4193a7a40d) which are vulnerable to the following economic attack described in the following [Medium post by Peter Zeitz](https://medium.com/@peter_4205/curve-vulnerability-report-a1d7630140ec).

**Recommendation:** Implement the recommendations indicated at the end of the blog post.

### QSP-3 No tests for geyser contracts

Severity: High
Affected files: snowswap_staking_geyser_1.sol, geyser_factory.sol

**Description:** Although the [Uniswap contracts](https://github.com/Uniswap/liquidity-staker/commit/3edce550aeeb7b0c17a10701ff4484d6967e345f) which these contracts fork from have tests, these tests were not adapted for the code modifications that were performed. Since we also did not receive any specification, there is no clear way to determine the intended functionality is correct.

**Recommendation:** Implement unit and integration tests for the newly added code and the modified code.

### QSP-4 OUSD token should not be used according to origin blog post

Severity: High
Affected files: geyser_factory.sol

**Description:** OUSD has been hacked on Nov 17th. This [blog post](https://medium.com/originprotocol/urgent-ousd-has-hacked-and-there-has-been-a-loss-of-funds-7b8c4a7d534c) written by one of the co-founders indicates: "We also strongly advise that you do not attempt to buy or sell OUSD at this time."

**Recommendation:** Avoid using OUSD in this contract or do not deploy it until the aforementioned blog post is updated to indicate that using OUSD is safe again.

### QSP-5 Denial of Service if reward is too high

Severity: Medium
Affected files: geyser_factory.sol

**Description:** The `require` statement inside the `StakingRewards.updateReward` modifier will cause the following functions to fail if the earned reward is higher than the current contract balance in `rewardToken`:
* `StakingRewards.stake`
* `StakingRewards.withdraw`
* `StakingRewards.getReward` called by `StakingRewards.exit`
* `StakingRewards.notifyRewardAmount`

This practically means that if the user's reward it too high they will not be able to stake, withdraw or exit at the moment they wish to do so. The user would have to wait for the balance of this geyser contract to be supplemented with extra `rewardToken`s before they can call any of the aforementioned functions successfully.

**Recommendation:** Even if the reward is higher than the contract balance, allow the user to call any of the aforementioned functions successfully and simply keep track of any residual rewards that the user is entitled to later when the balance of the geyser contract has been supplemented with extra `rewardToken`s.

### QSP-6 Inconsistent balance and transferred amounts

Severity: Medium
Affected files: geyser_factory.sol

**Description:** The `StakingRewards.stake` and `StakingRewards.withdraw` functions use different amounts for updating the `_balances` of the `msg.sender` and transferring from and to the message sender, respectively.
If the return value of `ousd.rebasingCreditsPerToken()` does not change between the call to `stake` and `withdraw` for the same user, then these differences will be complementary and the final amounts in the `_balances` and the `stakingToken` would be consistent.
However, if the value of `ousd.rebasingCreditsPerToken()` changes, then this could lead to inconsistencies which could affect other users.

**Exploit Scenario:**
The following sequence of steps show how such inconsistencies could occur:
1. The return value of `ousd.rebasingCreditsPerToken()` is `1e18` initially.
2. Alice stakes an amount of `100` staking tokens by calling `StakingRewards.stake(100)`. This leads to:
* Alice's balance being set to `100`.
* Alice transferring `100` stake tokens to the contract.
* The event `Staked(address(Alice), 100)` being emitted.
3. The return value of `ousd.rebasingCreditsPerToken()` is changed to `2e18` be calls to `ousd.changeSupply` by the vault.
4. Alice wants to withdraw `100` staking tokens by calling `StakingRewards.withdraw(100)`. This leads to:
* Alice's balance being set to `0`.
* The contract transferring `50` staking tokens to Alice.
* The event `Withdrawn(address(Alice), 100)` being emitted.
5. At this point Alice would be confused as to why she only received `50` staking tokens back. When she checks the emitted event she would see that the smart contract actually indicates `100` staking tokens were withdrawn.
6. We can continue this mental exercise with Bob wanting to stake `100` stake tokens to the contract at this point. This would lead to:
* Bob's balance being set to `200`.
* Bob transferring `100` stake tokens to the contract.
* The event `Staked(address(Bob), 100)` being emitted.
7. The return value of `ousd.rebasingCreditsPerToken()` is changed back to `1e18` be calls to `ousd.changeSupply` by the vault.
8. Bob notices that for some reason his balance is `200` after calling `StakingRewards.balanceOf(address(Bob))` and wants to withdraw `200` staking tokens by calling `StakingRewards.withdraw(200)`. This leads to:
* Bob's balance being set to `0`.
* The contract transferring `200` staking tokens to Bob if enough tokens are available. Otherwise the transaction would fail at this point and Bob would not be able to withdraw `200` tokens.
* The event `Withdrawn(address(Bob), 100)` being emitted.

**Recommendation:** Use consistent values for updating balances and making transfers.

### QSP-7 Missing input validation

Severity: Low
Affected files: geyser_factory.sol, snowswap_staking_geyser_1.sol

**Description:** The following functions do not perform input validation on the indicated parameters:
1. `StakingRewards.stake` does not check if the value of the `amount` parameter is greater than zero. Therefore staking an amount of zero would lead to wasted gas.
2. `StakingRewardsFactory.constructor()` does not check for non-zero `_rewardsToken`
3. `StakingRewardsFactory.deploy()` does not check for non-zero `stakingToken` and `rewardAmount` can be arbitrary.

The consequences of these missing input parameter checks vary, however, they may have a low impact on the security of the smart contract.

**Recommendation:** All input values originating from untrusted or error-prone human input, should be validated accordingly.

### QSP-8 Reliance on external smart contracts

Severity: Low
Affected files: geyser_factory.sol, snowswap_staking_geyser_1.sol

**Description:** The code relies on external contracts/tokens such as OUSD, which could themselves contain security issues. Such security issues were not in scope for this audit.

**Recommendation:** The Snowswap owner/maintainers must ensure these external contracts are vetted, such that they do not cause any issues for Snowswap users.

### QSP-9 Privileged roles and ownership

Severity: Info
Affected files: geyser_factory.sol, snowswap_staking_geyser_1.sol,

**Description:** Smart contracts will often have `owner` variables to designate the person with special privileges to make modifications to the smart contract. The following instances were identified in this project:
1. The owner of the `StakingRewardsFactory` contract from `geyser_factory.sol`, can transfer any amount of `rewardToken` to any address at any time, multiple times.
2. The owner of the `StakingRewardsFactory` contract from `snowswap_staking_geyser_1.sol,`, can transfer any amount of `rewardToken` to any address at any time, multiple times.

**Recommendation:** These centralization of power needs to be made clear to the users, especially depending on the level of privilege the contract allows to the owner.

### QSP-10 Unlocked pragma

Severity: Info
Affected files: snowswap_staking_geyser_1.sol, geyser_factory.sol

**Description:** Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.5.*`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked".

**Recommendation:** For consistency and to prevent unexpected behavior in the future, it is recommended to remove the caret to lock the file onto a specific Solidity version.

## Adherence to Specification

Quantstamp has not received any specification for this audit. However, the following functional requirements were derived from auditing the code:

1. A 0.5% vault fee is subtracted from the withdrawal amount only if the underlying balance is strictly less than the withdrawal amount (including the fees).

We ask the dev team to confirm or infirm each of the derived requirements from above.

## Code Documentation

The following issues have been identified in code comments:

1. The comment inside the `StakingRewards.updateReward` modifier in the `geyser_factory.sol` file is copied from the `StakingRewards.notifyRewardAmount` function. However, the code lines below this comment have been and do not match the comment anymore. We recommend updating the comment to match the updated implementation.
2. The comment "# integer maths" on L362 and L435 in `curve-contract-pool_yvault/vyper/stableswap.vy` does not explain the code on the following line. We recommend replacing this with a more descriptive comment.
3. The comment "# improved precision based on yVault contract logic" on L359 in `curve-contract-pool_yvault/vyper/stableswap.vy` is unclear and it might be referring to a commented code line on L360. We recommend clarifying this comment or removing it if it is not necessary.

## Best Practices

The following deviations from best practices were identified:

1. TODO comments in code should be addressed and removed. The following instances were identified in the code:
* "TODO: compare against yvault contract to see if there are any differences from yERC20" on L10 in `curve-contract-pool_yvault/vyper/yvERC20.vy`
* "YOLO 420 69 BAC1" on L2 in `curve-contract-pool_yvault/vyper/yvERC20.vy`
* "TODO: update these to take into account withdrawal fees" on L342 in `curve-contract-pool_yvault/vyper/stableswap.vy`
* "wtf is this? its never used" on L438 in `curve-contract-pool_yvault/vyper/stableswap.vy`

2. Commented code should be removed. The following instances of commented code were identified:
* "# r: uint256 = (yvERC20(self.coins[j]).balance * dy_) / yvERC20(self.coins[j]).totalSupply()" on L360 in `curve-contract-pool_yvault/vyper/stableswap.vy`

3. Magic numbers should be replaced by named constants, which have a meaningful name. Otherwise, the use of magic numbers reduces the maintainability and readability of the code. The following instances were identified:
* `5` on L363 in `curve-contract-pool_yvault/vyper/stableswap.vy`
* `1000` on L363 in `curve-contract-pool_yvault/vyper/stableswap.vy`
* `1e18` on L569 and L574 in `snowswap_staking_geyser_1.sol` assumes 18 decimal points per token, which does not hold for tokens such as USDC.
* `1e18` on L575, L580, L597, L603 and L622 in `geyser_factory.sol` assumes 18 decimal points per token, which does not hold for tokens such as USDC.

4. Unused variables should be removed. The `ok` variable on L438 in `curve-contract-pool_yvault/vyper/stableswap.vy` is not used an should be removed.