# Audit Notes

**Project name:** Request Network
**Auditor name:** Temitope Akinbosoye
**Commit hash used for auditing:** https://github.com/RequestNetwork/requestNetwork/blob/53ebc7a54a610ac28f616f87e1bda6f3d1e3265f/packages/smart-contracts/src/contracts/ERC20EscrowToPay.sol
<!-- **Commit hash:** 53ebc7a54a610ac28f616f87e1bda6f3d1e3265f -->
**List of spec/docs:**
- Get Started doc: https://docs.request.network/docs/guides/0-getting-started
- Escrow doc: https://support.request.network/essentials/escrow
- Whitepaper: https://request.network/assets/pdf/request_whitepaper.pdf

## Executive Summary

**Total number of issues found:** 12
**Severity distribution of issues:**

<style>
.borderless table thead {
display: none;
}
</style>

<div class="borderless">

| | |
| ------------- | ----- |
| High | 2 |
| Medium | 2 |
| Low | 3 |
| Informational | 5 |
| Undetermined | 0 |
</div>

## List of issues

| 1. | |
| ------------------ | ------------- |
| **Severity** | Informational |
| **Title** | Unlocked Pragma |
| **Description** | Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.8.*`. 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". |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 2 |
| **Exploit** | - |
| **Recommendation** | For consistency and to prevent unexpected behavior in the future, we recommend to remove the caret to lock the file onto a specific Solidity version. |

| 2. | |
| ------------------ | ------------- |
| **Severity** | Informational |
| **Title** | Event defined but never emitted |
| **Description** | Event `TransferWithReferenceAndFee` is defined but never used. Comment in line 334 suggests that it is expected to be emitted when payee is the receiver within `_withdraw` method. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 133 |
| **Exploit** | - |
| **Recommendation** | We recommend to remove or comment out `TransferWithReferenceAndFee` event if not needed. |

| 3. | |
| ------------------ | ------------- |
| **Severity** | Informational |
| **Title** | Event address parameters missing `indexed` attribute |
| **Description** | The `indexed` attribute allows search for the event using the indexed parameters as filters. Event parameters with `indexed` attribute will appear in the structure of topics, not the data portion of the log event. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 134, 135, 139 |
| **Exploit** | - |
| **Recommendation** | We recommend to add `indexed` attribute to address parameters that need to be searched in log events. At most, 3 parameters can have `indexed` attribute within an event. |

| 4. | |
| ------------------ | --- |
| **Severity** | Low |
| **Title** | Constructor first argument not validated |
| **Description** | The validity of parameter `_paymentProxyAddress` is not checked. If Null Address were to be wrongly passed by mistake, `paymentProxy` will be set to null and the smart contract will need to be re-deployed. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 142 |
| **Exploit** | - |
| **Recommendation** | We recommend the constructor address inputs be verified against null address to prevent an unnecessary re-deployment of the smart contract in case of mistake. |

| 5. | |
| ------------------ | ------------- |
| **Severity** | Informational |
| **Title** | Comments missing for external functions |
| **Description** | Comments are missing for functions `setEmergencyClaimPeriod` and `setFrozenPeriod`. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 154, 158 |
| **Exploit** | - |
| **Recommendation** | Proper source commenting is part of good documentation. We recommend that all `external` and `public` functions should have appropriate comments explaining what the functions are for. |

| 6. | |
| ------------------ | ------ |
| **Severity** | Medium |
| **Title** | Input parameters for `onlyOwner` external functions not validated |
| **Description** | The input parameters for `setEmergencyClaimPeriod` and `setFrozenPeriod` are not validated against expected range. The Request Escrow doc mentions minimum of 6 months for emergency claim and minimum of 12 months for freeze period but these are not checked in the code. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 154, 158 |
| **Exploit** | The owner (who is most likely the payer) can mistakely or knowly set wrong escrow emergency claim period or freeze period to his benefit or to the detriment of the payee. |
| **Recommendation** | In order to avoid payment disputes, we recommend verification of arguments of public functions confirming if they are within documented ranges. |

| 7. | |
| ------------------ | ---- |
| **Severity** | High |
| **Title** | Owner can withdraw fund even after receiving service |
| **Description** | If the owner is the payer, he can manipulate the contract to withdraw his fund quickly enough before the payee can realize it. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 158, 217, 316 |
| **Exploit** | The owner can follow below steps to withdraw his fund within few seconds. He can get his fund without paying the payee for service rendered.<br/>1. Call `setFrozenPeriod` to set freeze period to few seconds.<br />2. Call `freezeRequest` to lock the fund in the escrow.<br />3. Wait for the few seconds to elapse and then call `refundFrozenFunds` to withdraw the fund. |
| **Recommendation** | We recommend setting minimum limit for the freeze period that the escrow owner is able to set. The period should be long enough to punish or deter an unresponsive owner that wants to avoid payment. The Escrow doc mentions 12 months. |

| 8. | |
| ------------------ | ---- |
| **Severity** | Medium |
| **Title** | Owner can set long emergency claim period |
| **Description** | The input parameter for `setEmergencyClaimPeriod` is not limited to a maximum value. In theory, the owner can set it to the max value of `uint256` which is a long period. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 154 |
| **Exploit** | After receiving service, a malicious owner in bad faith can set the emergency claim period to very long period (like 1,000,000 weeks) much longer than what the payee can wait for. |
| **Recommendation** | We recommend having a reasonable maximum limit for the emergency claim period that the owner can set. The input parameter should be confirmed that it is not more than the maximum value. |

| 9. | |
| ------------------ | --- |
| **Severity** | Low |
| **Title** | Some `payEscrow` arguments are not checked |
| **Description** | `_tokenAddress`, `_to` and `_feeAddress` parameters of `payEscrow` are not validated. `payEscrow` is an external function that anyone can call. If Null Address were to be wrongly passed by mistake, there is no other way to update except creating another payment reference. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 175 |
| **Exploit** | - |
| **Recommendation** | Since `payEscrow` is an external funtion, all its arguments should be verified. At least, we recommend to verify that `_to` and `_feeAddress` are not Null Address in order to prevent wrong transfer of funds. |

| 10. | |
| ------------------ | ---- |
| **Severity** | High |
| **Title** | Local states modified after making external calls |
| **Description** | Allowance approval and fund transfer for `paymentProxy` are calling external contracts. These calls are done before assert and delete of the payment reference local states and mapping. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 353 - 370 |
| **Exploit** | A malicious contract can hijack control flow after an external call and cause re-entrancy attacks. |
| **Recommendation** | We recommend checks-effects-interactions pattern. This means, assert checks should be performed first, delete of payment reference mapping should be done next, and approving allowance and fund transfer should be called last. |

| 11. | |
| ------------------ | ------------- |
| **Severity** | Informational |
| **Title** | Usage of IERC20-approve is discouraged |
| **Description** | `erc20.safeApprove()` internally calls IERC20 approve method in line 42 of `./lib/SafeERC20.sol`. The usage of IERC20-approve is discoraged. This is why `safeApprove` method is already deprecated in OpenZeppelin contract. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 382 |
| **Exploit** | Changing an allowance with IERC20-approve method brings the risk that an attacker can use both the old and the new allowance by front-run transaction race condition. See attack vector doc [here](https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/) for more details. |
| **Recommendation** | We recommend to use `safeIncreaseAllowance` of OpenZeppelin instead. |

| 12. | |
| ------------------ | --- |
| **Severity** | Low |
| **Title** | Approved token allowance is too much |
| **Description** | Maximum value of `uint256` of the payment token is approved to the payment proxy. This is more than the request amount. |
| **File(s)** | ERC20EscrowToPay.sol, line(s): 381, 382 |
| **Exploit** | Race condition issue in ERC20 approve method can be exploited by a malicious payment proxy to spend all approved allowance which will be more than fund amount. |
| **Recommendation** | We recommend to only approve the payment amount, not maximum possible value. |

## Documentation issues

- Only [ERC20EscrowToPay.sol](https://github.com/RequestNetwork/requestNetwork/blob/53ebc7a54a610ac28f616f87e1bda6f3d1e3265f/packages/smart-contracts/src/contracts/ERC20EscrowToPay.sol) source file was reviewed as part of this audit. No documentation was audited.

## Best practices

- Library `SafeERC20.sol` included in `ERC20EscrowToPay.sol` (line 6) was self-created by the author. It can be replaced with `@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol` which is a similar library from OpenZeppelin. OpenZeppelin contracts are well audited and extensively battle-tested.