# Dexpools Audit Notes

**Auditor:** Jake Bunce

**Client:** Dtrade https://dexpools.com/

https://github.com/Dexpools/smart-contracts

Git commit SHA: `749ee2e`

## Whitepaper & specification about the project

Whitepaper: https://dexpools.com/whitepaper/

Project is a decentralised P2P OTC platform.

## Review of the protocol/implementation

**[1] Unlocked Pragma**

**Files Affected:** `*.sol`

Default AL text. If using `>=0.8.0` there's no requirement to import OZ SafeMath.

**[2] Loop could exhaust gas for a large number of addresses**

I think there's a default AL text for this?

**Files Affected:** `BalanceScanner.sol`

**Severity: Informational**

[`etherBalances()`](https://github.com/Dexpools/smart-contracts/blob/main/contracts/BalanceScanner.sol#L25) takes an argument of `addresses`. If this array is very large the function may never complete, reverting with "Out of gas during execution".

Same applies to [`tokenBalances`](https://github.com/Dexpools/smart-contracts/blob/main/contracts/BalanceScanner.sol#L40), [`tokensBalance`](https://github.com/Dexpools/smart-contracts/blob/main/contracts/BalanceScanner.sol#L55)

**Recommendations:**

Consider adding an upper bound for the array size to ensure this function always is executable.

**[3] staticCall usage fixed gas**

**Files Affected:** `BalanceScanner.sol`

**Severity: Low**

Functions that have the amount of gas to use fixed in code are susceptible to conditions where the total required for execution can change between upgrades or forks or the Ethereum blockchain. This can result in functions failing that previously worked after an upgrade as the required gas utilisation has changed but the value is fixed in code.

**Recommendations:**

Consider whether a call directly to the token contract for balances could be implemented instead of using this proxy read state.

**[4] Contract can be left without an owner**

I wonder if we could get this added as a new AL default text? It comes up often.

**Files Affected:** `Owner.sol`

**Severity: Low**

The contract could be left without an owner if `renounceOwnership()` is called.

**Recommendations:**

Consider overriding the function `renounceOwnership()` with a transaction that reverts.

**[5] Inconsistent RBAC implementation**

**Files Affected:** `Owner.sol`, `DXP.sol`

**Severity: Low**

`DXP.sol` uses RBAC for role based access in the ERC20 contract, however `Owner.sol` uses an `OnlyOwner` modifier to prevent unauthorised calling of the function.

**Recommendations:**

Consider using a single approach to access control that is consistent across the project.

**[6] Time value does not make sense**

**Files Affected:** `Owner.sol`

**Severity: Medium**

The value of `defaultPendingTime` on deployment is set to 6. It is not clear whether this is minutes, hours, days, or weeks.

**Recommendations:**

Change this value to be seconds since the Unix epoch so the time value is clear.

**[7] Missing validation check in setPairFee()**

**Files Affected:** `Owner.sol`

**Severity: Low**

[`setPairFee`](https://github.com/Dexpools/smart-contracts/blob/main/contracts/Owner.sol#L69) does not check whether the addresses for `token0` and `token1` are the same. Consequently a pair could be created mistakenly for the same asset.

**Recommendations:**

Consider adding a check in this function `require(token0 != token1, "Pair cannot be the same asset");`

## Best Practices

**[1] Lint Errors**

TransactionManager.sol
  231:2  error  Line length must be no more than 120 but current length is 128  max-line-length
  253:2  error  Line length must be no more than 120 but current length is 128  max-line-length
  300:2  error  Line length must be no more than 120 but current length is 128  max-line-length
  322:2  error  Line length must be no more than 120 but current length is 128  max-line-length

✖ 4 problems (4 errors, 0 warnings)

**[2] Missing docstrings for desired function outcomes**

Example: [`depositUser0`](https://github.com/Dexpools/smart-contracts/blob/main/contracts/TransactionManager.sol#L135)

**[3] Repetition of code**

Deposit functions in `TransactionManager.sol` have the same code repeated in two separate deposit functions. It would be better to have a single function and denote whether this is for depositor0 or depositor1 as an argument. Same applies to `withdrawUser` functions.