* Project name: Request Escrow * Auditor name: Joshua Evuetapha * The commit hash used for auditing: https://github.com/RequestNetwork/requestNetwork/blob/53ebc7a54a610ac28f616f87e1bda6f3d1e3265f/packages/smart-contracts/src/contracts/ERC20EscrowToPay.sol * List of spec/docs: https://support.request.finance/essentials/escrow * List of issues 1. * Severity: High * Title: Potential Malicious Code with External Contract. * Description: In Solidity any address can be casted into specific contract, even if the contract at the address is not the one being casted. * File(s): ERC20EscrowToPay.sol (Line 142 -144). * Exploit (optional): - This contract may be initialized with a malicious code at the constructor. * Recommendation: The address of external contract should be verified and audited. 2. * Severity: High * Title: Reentrancy * Description: A malicious contract can reenter the `payRequestFromEscrow` function and make multiple withdrawals. * File(s): ERC20EscrowToPay.sol (Line 240 - 248). * Exploit (optional): - * Recommendation: Use function modifiers that prevent re-entrancy. 3. * Severity: High * Title: FeeAddress and FeeAmount can be manipulated. * Description: The `payEscrow` function accept `feeAddress` and `feeAmount` as arguments, this can lead to users setting any address receive fees, they can also set zero `feeAmount` thereby paying no fees. * File(s): ERC20EscrowToPay.sol (Line 175 -208). * Exploit (optional): - * Recommendation: Fees address should be a state variable in the smart contract that only owner can set, and the fees amount should be a percentage of the `_amount`. 4. * Severity: Medium * Title: No validation for `_tokenAddress` and `to` Addresses * Description: There is a possibility of passing `address(0)` as the `_tokenAddress` or `_to` address as parameters to the `payRequestFromEscrow` function . * File(s): ERC20EscrowToPay.sol (Line 175 - 208). * Exploit (optional): - * Recommendation: The `_tokenAddress` and `to` should be checked to make sure they are not `address(0)`. 5. * Severity: Low * Title: Transfering zero tokens to `address(0)` * Description: Line 362 and 363 attempts to send 0 tokens to `address(0)`, this is a redundant operation that will cost more gas than needed. * File(s): ERC20EscrowToPay.sol (Line 348) * Exploit (optional): - * Recommendation: Create a new function that transfers tokens to the payee only. 6. * Severity: Informational * Title: Unlocked Pragma * Description: Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.8.4`. 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 * Exploit (optional): - * 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. 7. * Severity: Informational * Title: Ignored return value from interaction with an external contract * Description: The return value of `erc20.safeApprove(address(paymentProxy), max)` was ignored this could lead to unexpected behaviour. * File(s): ERC20EscrowToPay.sol (Line 348) * Exploit (optional): - * Recommendation: Get the return value from that interaction, if it's true proceed, else revert with an error * Documentation issues (section) * The `feeAddress` and `feeAmount` were not adequately discussed in the documentation, how is the `feeAmount` generated and who own's the `feeAddress`. * The functions `setEmergencyClaimPeriod` and `setFrozenPeriod` were not documented.