# VHS Security Analysis & Review
A security analysis review of the Cryptofield-core repo was done for [VHS](https://www.vhslab.com/)
This final audit report includes all the vulnerabilities, issues and code improvements found during the security reviews and post-fix status.
## Benchmark Guidelines
### 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
- **High** - client **must** fix the issue.
- **Medium** - client **should** fix the issue.
- **Low** - client **could** fix the issue.
## Executive summary
### Overview
| | |
| :------------ | :------------------------------------------------------------------------------------------- |
| Project Name | Cryptofield-core |
| Repository | https://github.com/vhslab/cryptofield-core/tree/c2b5595315cfd806ca921aca81203dd52521f28e |
|Scope|contracts/proxy_deployment/tournaments/BuyInTournaments.sol contracts/proxy_deployment/game_items/GameItems.sol|
| Commit hash | [c2b5595315cfd806ca921aca81203dd52521f28e](https://github.com/vhslab/cryptofield-core/tree/c2b5595315cfd806ca921aca81203dd52521f28e)|
|Auditors|[Tirtha S.](https://twitter.com/sarkerium), [Parth P.](https://twitter.com/__parthpatel__)|
| Methods | Manual review |
| |
### Issues found
| Severity | Count |
| :------------ | --------------------------------------------------------
| High risk | 5| 7 |
| Medium risk | 5 | 2|
| Low risk | 0 | 2|
| Informational | 2 | 4|
| Gas | 2| |
### Vulnerability List
| Id | Description | Status |
|:------- |:---------------------------------------------------------------------------------------------------:| ------------ |
| **H-1** | All GameItems NFT transfers will fail due to `_beforeTokenTransfer` hook | *FIXED* |
| **H-2** | Burning of token is possible even if contract is `paused` | *FIXED* |
| **H-3** | `getTotalCost` function returns invalid values | *FIXED* |
| **H-4** | `ownerOf` horseId can return address(0) during `registerHorseWithSignature`. | *FIXED* |
| **H-5** | Horses can be registered multiple times for same tournament id. | *FIXED* |
| **M-1** | The contract is not paused by default. | *FIXED* |
| **M-2** | The imported contract `EIP712MetaTransaction` is not upgradeable. | *ACKNOWLEDGED* |
| **M-3** | The `MINTER_ROLE` can pass duplicate id in the `mint` function to mint more than `maxTransferLimit` | *FIXED* |
| **M-4** | `supportsInterface` will return false for interfaceid of `AccessControlUpgradeable` | *FIXED* |
| **M-5** | Entrant fee is taken from different account and refund is done to different account. | *FIXED* |
| **I-1** | Redundant `require` statement in `_beforeTokenTransfer` hook | *FIXED* |
| **I-2** | Unused imports | *FIXED* |
| **G-1** | Replace require with custom errors | *ACKNOWLEDGED* |
| **G-2** | Cache length of list when using loops | *FIXED* |
# Findings
## High severity
### [H-1] All GameItems NFT transfers will fail due to `_beforeTokenTransfer` hook
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L566
#### **Description**
The hook `_beforeTokenTransfer` uses below condition to check whether either of `from` or `to` address should be `address(0)` for NFT transfer. This condition won't be true for normal NFT transfers and will always revert for NFT transfers.
```
require(from == address(0) || to == address(0), "GameItems: from and to are both zero");
```
#### **Recommended Mitigation Steps**
Modify the condition according to the project requirement.
#### Resolution
Necessary action has been taken.
### [H-2] Burning of token is possible even if contract is `paused`
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L214
#### **Description**
The `pause` functionality is used to pause `minting` and `burning` of NFTs. For minting, `whenNotPaused` is used pausing the minting functionality. However, for burning, this is not used causing the `burn` to function correctly even if the contract is paused.
#### **Recommended Mitigation Steps**
Add `whenNotPaused` modifier in the `burn` functionality.
#### Resolution
Necessary action has been taken.
### [H-3] `getTotalCost` function returns invalid values
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L244
#### **Description**
`getTotalCost` function takes array of `ids` and `amounts` and return the total cost of minting. However, the token's payment ERC20 token can be different for different id and it doesn't make sense to add them directly. Thus, value returned by `getTotalCost` is not valid.
#### **Recommended Mitigation Steps**
Refactor the function's design.You may want to take in account of the different value of tokens.
#### Resolution
Necessary action has been taken.
### [H-4] `ownerOf` horseId can return address(0) during `registerHorseWithSignature`.
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/tournaments/BuyInTournaments.sol#L120
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/tournaments/BuyInTournaments.sol#L160
#### **Description**
`registrationApprover` can approve the registration of horse with specific owner. It can be possible that horseId gets burned and `ownerOf` returns `address(0)` while `registerHorseWithSignature`. This horse will get resigtered and entrance fee will be taken from `_msgSender()` but won't be possible to deregister horse from tournament due to following check in `deregisterHorseFromBuyInTournament`.
```
require(tournamentEntries[tournamentId][horseId].nominator != address(0), "Horse not registered to tournament"); //@audit this can be zero and then it is not possible to deregister
```
#### **Recommended Mitigation Steps**
Add a check reverting the transaction when `ownerOf` horse to be registered is `address(0)`at L121
#### Resolution
Necessary action has been taken.
### [H-5] Horses can be registered multiple times for same tournament id.
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/tournaments/BuyInTournaments.sol#L140
#### **Description**
There is no check to ensure that only a single `horseId` can be registered per tournament. It is possible to register same `horseId` multiple times for same tournament. Also, `tournamentHorsesIndices` mapping will be overrided due to multiple `horseId` registration for same tournaments.
#### **Recommended Mitigation Steps**
Ensure that same `horseId` shouldn't be registered for same tournament. You can add something like the following on L139.
```
require(tournamentEntries[tournamentId][horseId].nominator == address(0), "Horse already registered in tournament");
```
#### Resolution
Necessary action has been taken.
### [M-1] The contract is not paused by default.
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L33
#### **Description**
In L33 of the Natspec doc, the assumption is that contract will paused by default and only be unpaused by admin. However, in the `constructor` of GameItems, there is no call to `pause` the contract which leaves it as unpaused by default.
#### **Recommended Mitigation Steps**
Call the `_pause` method in constructor to pause the contract by default or update the documentation.
#### Resolution
Necessary action has been taken.
### [M-2] The imported contract `EIP712MetaTransaction` is not upgradeable.
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L46
#### **Description**
The `GameItems` inherit from different contracts like `Initializable`, `ERC1155Upgradeable`, `AccessControlUpgradeable`, `PausableUpgradeable`, `ReentrancyGuardUpgradeable`, `EIP712MetaTransaction`. All other contracts are `upgradeable` except `EIP712MetaTransaction`. Also, it does not reserve any storage gaps.
#### **Recommended Mitigation Steps**
Consider following OpenZeppelin guide for writing [upgradeable smart contracts](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable).
#### Resolution
The team informed: "EIP712MetaTransactiwill be used as is to maintain consistency with other Zed Run related contracts.". So the import remains as it is.
### [M-3] The `MINTER_ROLE` can pass duplicate id in the `mint` function to mint more than `maxTransferLimit`
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L176
#### **Description**
The `maxTransferLimit` is used to ensure that in one transaction, no recipient should get NFTs more than `maxTransferLimit`. However, there is no check in smart contracts for duplicate recipient id. It is possible to mint more than `maxTransferLimit` for same NFT Id if there are duplicates present in the `ids` array.
#### **Recommended Mitigation Steps**
Add a check in smart contract to ensure that there are no duplicate ids in `mint` function.
#### Resolution
The team provided solution will ideally work when `ids` array is in sorted order. They are maintaining a simple validation to save gas.
### [M-4] `supportsInterface` will return false for interfaceid of `AccessControlUpgradeable`
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L599
#### **Description**
`supportsInterface` will return false for interfaceid of `AccessControlUpgradeable` because that is not taken into account.
#### **Recommended Mitigation Steps**
Change the return value as follows:
```
super.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId)
```
#### Resolution
Necessary action has been taken.
### [M-5] Entrant fee is taken from different account and refund is done to different account.
#### **Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/tournaments/BuyInTournaments.sol#L264
#### **Description**
It is possible that horse is not borrowed at time of registration and entrant fee is taken from horse owner. Now, if horse is borrowed by some other account and horse is deregistered, refund will be sent to different account that the one who paid the entrant fee. Similar situation will arise when horse is transferred to some other account or a switch of borrower occurs.
#### **Recommended Mitigation Steps**
Consider various cases and handle it accordingly.
#### Resolution
Necessary action has been taken.
## Informational
### [I-1] Redundant `require` statement in `_beforeTokenTransfer` hook
**Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L565
**Description**
In hook `_beforeTokenTransfer` , the following require statement is redundant. Because this condition will already be checked in `_mintBatch` function.
```
require(ids.length == amounts.length, "GameItems: ids and amounts length mismatch");
```
**Recommendation** Remove the require statements.
#### Resolution
Necessary action has been taken.
### [I-2] Unused imports
**Context**
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L13
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L16
- https://github.com/vhslab/cryptofield-core/blob/c2b5595315cfd806ca921aca81203dd52521f28e/contracts/proxy_deployment/game_items/GameItems.sol#L15
**Description**
In `GameItems`, `ContextUpgradeable`, `IERC20` and `Context` are imported but not used.
**Recommendation:** Remove unused imports.
#### Resolution
Necessary action has been taken.
## Gas Optimization
### [G-1] Replace require with custom errors
**Description** In both `GameItems.sol` & `BuyInTournaments.sol` require statements have been used for error handling - which is gas intensive and increases with the length of the error string.
**Recommendation:** Replace `require` statements with custom error for better gas optimization.
#### Resolution
Continue with the existing format.
### [G-2] Cache length of list when using loops
**Description** In both `GameItems.sol` & `BuyInTournaments.sol` wherver for loop has been implemented, the iterator length is calculated on each loop.
e.g:
In L175 of `GameItems.sol`, we are calculating the `ids.length` on each iteration.
**Recommendation** Store the iterator length in a local variable and replace that in the for loops. It will save the unnecessary recalculation expense.
e.g:
```
uint256 idLength = ids.length;
for (uint256 i = 0; i idLength; ++i) {
...
```
#### Resolution
Necessary action has been taken.
---
## Disclaimer
The smart contracts submitted for audit have been meticulously evaluated in accordance with the prevailing industry standards as of the date of this report, encompassing cybersecurity vulnerabilities and concerns within the smart contract source code, the particulars of which are divulged herein (hereinafter referred to as the "Source Code"); the Source Code compilation, deployment, and functionality (executing the intended functions). This report expressly disclaims any representation or warranty as to the comprehensive identification of all vulnerabilities or the complete security of the code. The scope of this report is limited to the code that has been provided for review, and therefore may not be pertinent to any subsequent modifications. It is important not to construe this report as a conclusive and exhaustive evaluation with respect to the utility, safety, freedom from defects, or any other contractual representations pertaining to the code.
Sign in with Wallet
Connect another wallet