# Jake's Audit Notes: Zed

Client: https://zed.run/
Repo: https://github.com/vhslab/cryptofield-core
Commit: `a4f916ec225699faf40d585bc06daa2d4a0c7c5a`

## Whitepaper & specification about the protocol

No whitepaper on https://zed.run or Jira.

Background on the project at https://community.zed.run/welcome-to-zed/

"The mission of ZED is to create an ecosystem where collectible digital assets hold value and have use in an exciting and ever evolving gaming and wagering environment."

Project is a horse breeding/racing game where users compete in digital races.

## Review of the protocol/implementation

### Core.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/Core.sol

[1] **Possible to predict horse gender**

[`_getRandGender()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/Core.sol#L266) uses the previous block number modulo 2 to determine the gender. If the block number is divisible by 2 the horse is a male. If there is a remainder the horse is a female. If a user wants a specific horse for breeding it is possible to time the transaction to enable the desired gender outcome. Moreover this results in a 35% discount during breeding if a user has both male and female horses.

[2] **Update of Admin role**

[`grantRoleAdmin()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/Core.sol#L489) updates the access control to enable a different address to assume the role. An event should be emitted for monitoring occurences of this happening in production.

[3] **Missing docstrings**

[`getBaseValue()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/Core.sol#L242) does not have a docstring

### BreedTypes.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/BreedTypes.sol

[1] **Event update of Admin role**

[`grantRoleAdmin()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/BreedTypes.sol#L182) updates the access control to enable a different address to assume the role. An event should be emitted for monitoring occurences of this happening.

### GOPCreatorV3.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPCreatorV3.sol

[1] **Lack of supporting documentation**

Not immediately clear from the code, website, or a Google search what a GOP is. It looks like this contract can receive funds from users in exchange for a horse.

[2] **Inconsistent behaviour between same functions**

`receiveGOPFunds()` in [`GOPCreatorV3.sol`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPCreatorV3.sol#L50) and [`GOPFundsReceiverWETH.sol`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPFundsReceiverWETH.sol#L57) appear to have the same functionality of receiving funds in exchange for a horse. The WETH receiver has a larger number of arguments relating to business logic parameters (such as horse price), which suggests to me that there is an inconsistency in the implementations between ETH/WETH.

[3] **Misuse of horse sale operation in withdraw function**

[`receiveGOPFunds()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPCreatorV3.sol#L50) is missing an access modifier which means anybody may call this function publically.

The `_horseCode` check may be bypassed if the attacker can determine the numbering scheme in use and force the contract to send Ether to `_fundsReceiver`. This creates a DoS condition where `_horseCode` can be marked as bought incorrectly by an attacker, making it appear that all horses have been purchased.

Recommendations:

[a] Implement a check to validate the amount of Ether sent in `msg.value` fulfils the price of the horse purchase before setting `isCodeUsed[_horseCode] = true;`

[4] **Multiple instances of events missing for Admin tasks**

For example, in [`changeFundsReceiver()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPCreatorV3.sol#L152) there is no event emitted when the fund receiver has been updated. This is useful for monitoring activity in production when admin accounts are changed.

### GOPFundsReceiverWETH.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPFundsReceiverWETH.sol

[1] **Admin address unable to receive funds from sales**

[`changeFundsReceiver()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/GOPFundsReceiverWETH.sol#L114) is missing the `payable` keyword, so the contract will not be able to send funds from horse sales to the intended admin.

### HorseData.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/HorseData.sol

[1] **Function appears to be incomplete**

[`getGenotype()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/HorseData.sol#L74) appears to be an incomplete implementation. The docstring states: `integer identifying the genotype` for a return value, however this lookup is not performed. The only functionality this provides is to return the same `unit256` as passed as a function argument.

[2] **Missing docstring on function**

[`getRandom()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/HorseData.sol#L120) is missing a docstring to describe the behaviour.

### StudServiceV5.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol

[1] **Seconds used for the duration of a day**

[`_defaultDuration`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L22) uses `86400` seconds to represent a day. Care should be taken in the event of leap seconds which can increase or decrease the number of seconds in a day. If this variable is used for any time based operations in the future the computation may not be accurate. The same applies to [these lines.](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L73-L75)

[2] **Multiple instances of an incorrect modifier on Admin functions**

[`adminPutInStud()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L386) uses the modifier `onlyOwners()`, which indicates that an only an admin should only be able to call the function. With `onlyOwners()` any user with the `STUD_OWNERS_ROLE` and a `_horseId` may call this.

The same applies to [`setBreedTypesAddress()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L398), [`setCoreAddress()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L408), [`setBreedTypeWeight()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L419), [`setBloodlineWeight()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L428), [`setDefaultDuration()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L436), [`setBaseFee()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L444), [`addTimeFrame()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L453), [`removeTimeFrame()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/normal_deployment/StudServiceV5.sol#L465)

Recommendation: Use the `onlyOwnersAdmins()` modifier to limit access to an admin and not a horse owner with `STUD_OWNERS_ROLE`.

### WinningsWatcher.sol

[1] **Any user can transfer WETH from the contract**

Presumably the operator of the service should be responsible for calling `sendWinnings()`. If this is correct then any function calls that are intended for the operator to execute should be restricted to that party only. Currently anybody may call this function and transfer WETH from the contract to themselves. No checks are performed on the provided arguments to validate the race or horse IDs are valid.

Recommendations:

[a] Implement an access modifier to only allow this function to be called by the operators of the service.
[b] Implement checks on the arguments provided to ensure valid race and horse IDs have been passed.

### Breeding.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/Breeding.sol

[1] **Unprotected constructor**

[`initilize()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/Breeding.sol#L27) is not protected with an access modifier. This means that after the deployment of the contract the function could be forgotten to be deployed or an attacker could assume the `BREEDING_OWNERS_ADMIN_ROLE`. Additionally other contracts in the project use `constructor()` and this is an inconsistency.

Recommendation:

[a] Use `constructor()` to define the constructor and pass the arguments in at deploy time to ensure the defined admin role for the admin address.

[2] **Misleading comment**

[`_getBaseValue()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/Breeding.sol#L374) passes integers 34 and 65 into `_getRandNum()` but the comment states `Creates a random number between 30 and 65`.

[3] **Commented code can be removed**

[`migrateData()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/Breeding.sol#L494) has multiple instances of commented out code. This can be removed.

### BreedingProxy.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/BreedingProxy.sol

None

### BreedingStorage.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/breeding/BreedingStorage.sol

None

### RacingArena.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/racing/RacingArena.sol

[1] Multiple incorrect access modifiers

[`adminRegisterHorse()`](https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/racing/RacingArena.sol#L336) uses the access modifier `onlyOwners()`, but this function appears to be an admin function which should use the admin modifier. Also applies to `changeFeeReceiver()`, `pause()`, `unpause()`.

Recommendations:

[a] Use the `onlyOwnersAdmins()` modifier.

### RacingProxy.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/racing/RacingProxy.sol

None

### RacingStorage.sol

https://github.com/vhslab/cryptofield-core/blob/master/0.7-contracts/proxy_deployment/racing/RacingStorage.sol

None

### General Comments

- Are horses intended to be immortal? This implementation does not appear to implement a lifetime feature.
- The compiler warns about duplicate naming of an interface named InterfaceCore. During the unit test execution a transaction fails with `Revert reason: Core: name already taken`. The duplicate naming should be resolved. Tests don't account for this and they pass with `91 passing (14m)`.
- Shouldn't use versioning in filenames or contract definitions. Use Github releases as there are inconsistencies between some files being versioned and some not.
- Missing docstrings common throughout the project.
- Events are often missing for admin actions of updates/pause/unpause which are useful to monitor in production.