**Project name**: Request Network
**Auditor name**: Uzoma jesse
**Commit hash used for auditing**: https://github.com/RequestNetwork/requestNetwork/blob/53ebc7a54a610ac28f616f87e1bda6f3d1e3265f/packages/smart-contracts/src/contracts/ERC20EscrowToPay.sol

**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: 7
Severity distribution of issues:**

| *High* | 3 |
| -------- | -------- |
| Medium | 1 |
| Low | 3 |
|Informational | |
|Undetermined||

List of issues
---

| 1 | |
| -------- | -------- |
| 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.

| 2 | |
| -------- | -------- |
| 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.

| 2 | |
| -------- | -------- |
| Severity | *High*|
Title|Block Timestamp Manipulation
Description|Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion section for further details), locking funds for periods of time and various state-changing conditional statements that are time-dependent. Miner's have the ability to adjust timestamps slightly which can prove to be quite dangerous if block timestamps are used incorrectly in smart contracts.
file(s)| ERC20EscrowToPay.sol, line(s): 231,264,282,323 - 370|
Exploit|block.timestamp or its alias now can be manipulated by miners if they have some incentive to do so. Let's construct a simple game, which would be vulnerable to miner exploitation.
Recommendation|If the scale of your time-dependent event can vary by 15 seconds and maintain integrity, it is safe to use a block.timestamp<br> This means if your contract does not rely on an interval of less then 15 seconds then it is okay to use block.timestamp..

.

| 3 | |
| -------- | -------- |
| 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..

| 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.

| 4 | |
| -------- | -------- |
| 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): 142|
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.

| 4 | |
| -------- | -------- |
| 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.

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
- The time difference between events can be estimated using block.number and the average block time. However, because block times can change and break functionality, it's best to avoid its use.