# Parcel(payroll) Security Review

A security review of the [Parcel](https://github.com/ParcelHQ/parcel-payroll) 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 | Parcel-payroll |
| Repository | https://github.com/ParcelHQ/parcel-payroll |
| Commit hash | [844f015a6e2ac598d718922ef6803eb2cbef723a](https://github.com/ParcelHQ/parcel-payroll/tree/844f015a6e2ac598d718922ef6803eb2cbef723a) |
| Documentation | [docs](https://parcelhq.notion.site/Parcel-Payroll-SC-Documentation-539f9a1a75a541a79b8f5809b66b62e1) |
| Methods | Manual review |
| |

### Issues found

| Severity | Count
| :------------ | --------------------------------------------------------
| High risk | 6| 7 |
| Medium risk | 3 | 2|
| Low risk | 1 | 2|
| Informational | 3 | 4|

# Findings

## High severity

### [H-1] All the approvers needs to reapprove if some nonce hasn't got required approval initially.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L92-L103

#### **Description**

If required approval is 4 and `payoutNonce` only received 3 approval, then the payout will not be processed for that nonce. Now, if another approver approves that nonce in different transaction, payout will still not be processed because approvals for that nonce will be `1`. This doesn't take into account that there were `3` approvers initially.

#### **Recommended Mitigation Steps**

Needs design change and refactoring.

### [H-2] Anyone can fetch ether from the Safe without providing any signatures

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L101-L102
- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L191-L197

#### **Description**

Any address can interact with `executePayroll` and fetch all the approved ethers from safe.

#### **Recommended Mitigation Steps**

Do the sanity check on the ether and check if there isn't any `fetched` funds left in the contract.

### [H-3] Frontrunners can frontrun `executePayroll` and make valid transaction to fail.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L92-L103

#### **Description**

Frontrunner can pull all ether from safe and if `executePayroll` contains `ether` as `paymentToken`, then the contract won't be able to receive ether and the valid transaction will always fail, ethers will get locked into the contract and further execution of payroll won't be processed.

#### **Recommended Mitigation Steps**

As this is creating more vulnerabilities, the better approach would be to follow modular approach separating pulling the payment by the approvers first and then executing the transaction.

### [H-4] Contract won't be able to receive ether.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/Organizer.sol#L15

#### **Description**

`Organizer` contract doesn't have any functions to receive ether into the contract. Due to this, `executePayroll` will always fail.

#### **Recommended Mitigation Steps**

Add `receive` function for adding the functionality in contract to `receive` ether.

### [H-5] Not following checks-effects-interactions may lead to reentrancy bugs

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L180-L181
- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L185-L186

#### **Description**

Not following checks-effects-interactions pattern may lead to serious reentrancy bugs as contract are interacting with unknown `erc20` tokens. More [info](https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html).

#### **Recommended Mitigation Steps**

Use checks-effects-interactions pattern while dealing with calling external contract.

### [H-6] `getPayoutNonce` returns wrong value

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L46-L57

#### **Description**

`getPayoutNonce` checks if `packedPayoutNonces.length < slotIndex` and returns false if this condition is satisfied. However, it should also return false if `slotIndex == packedPayoutNonces.length`. Because if `packedPayoutNonces.length` is 4, valid `slotIndex` will be `0,1,2,3`.

#### **Recommended Mitigation Steps**

Change the condition to `slotIndex <= packedPayoutNonces.length`.

## Medium severity

### [M-1] `packPayoutNonce` will only work for nonces increased linearly.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L26-L39

#### **Description**

If the nonce are generated non-linearly(randomly), then `packPayoutNonce` will revert because of following condition.

if (slot >= packedPayoutNonces.length) {
            packedPayoutNonces.push(1);
        }

#### **Recommended Mitigation Steps**

Only use linearly increased nonce.

### [M-2] Usage of `transfer` for sending ether is discouraged.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L180

#### **Description**

If gas costs are subject to change, then smart contracts can’t depend on any particular gas costs.

Any smart contract that uses `transfer()`` or `send()`` is taking a hard dependency on gas costs by forwarding a fixed amount of gas: `2300`.

More info [here by Consensys Diligence](https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/)

#### **Recommended Mitigation Steps**

Stop using `transfer()`` and `send()`` in your code and switch to using `call()`` instead.

### [M-3] `validatedRoots` array will always be `true`.

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L111
- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L164

#### **Description**

`validatedRoots` array members is set to `true` if it passes `require` check. If check doesn't passes, transaction will revert and hence this array has no significance on the code.

#### **Recommended Mitigation Steps**

Remove this array and comparisons with it.

## Low severity

### [L-1] No use of functionality for `false` flag in `packPayoutNonce`

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L26
- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L36-L38

#### **Description**

There is no functionality in the code which sets the `flag` to false. Hence, there is no need for `flag` argument and code corresponding to `false` flag is irrelevant.

#### **Recommendations**

Remove the `flag` variable in method signature and code corresponding to `false` flag.

## Informational

### [I-1] Different solidity compiler version pragma in different file.

#### **Description**

Some files uses version `^0.8.0` while some files has version `^0.8.9`.

#### **Recommendations**

Consider using fixed version pragma across all files.

### [I-2] Double check of condition in `if` block

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L174
- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/payroll/PayrollManager.sol#L50

#### **Description**

The condition is checked two times in the same `if` block.

#### **Recommendations**

Remove the condition as it's already checked in the function call.

### [I-3] Redundant variable

#### **Context**

- https://github.com/ParcelHQ/parcel-payroll/blob/844f015a6e2ac598d718922ef6803eb2cbef723a/contracts/signature/Signature.sol#L47

#### **Description**

The variable is not needed since the `payroll.rootHash` is same as function argument `rootHash`.

#### **Recommendations**

Remove the variable.