# Security Audit of `Narfex-fiat-factory` contracts.
## Conclusion
This audit was made by
Auditor: Vladimir Smelov <vladimirfol@gmail.com>.
Date: 2023-05-04
TODO
## Scope
TODO
## Methodology
1. Blind audit. Understand the structure of the code without reading any docs.
2. Ask questions to developers.
3. Run static analyzers.
4. Find problems with:
- backdoors;
- bugs;
- math;
- potential leaking of funds;
- potential locking of the contract;
- validate arguments and events;
- others.
## Result
#### CRITICAL-1.
At
- NarfexLawyers.sol:123
```solidity=123
if (getIsLawyer(_account)) return false;
```
should be "if (!getIsLawyer(_account)) return false;"
##### Status.
NEW
______
#### CRITICAL-2.
At
- NarfexP2pRouter.sol:50,63-64
```solidity=50
function swapToETH(address to, address token, uint amount) external {
```
```solidity=63
data.outAmount = 0;
data.outAmountMin = 0;
```
minAmountOut is absolutely requiring otherwise sandwich attack.
##### Status.
NEW
______
#### CRITICAL-3.
At
- NarfexExchangerPool.sol:150
```solidity=150
0,
```
sandwich attack
##### Status.
NEW
______
#### CRITICAL-4.
At
- NarfexExchangerRouter3.sol:68,257-268,272-280,354-357
```solidity=68
uint transferFee;
```
```solidity=257
if (data.isExactOut) {
/// Increase output amount by outgoing token fee for calculations
data.outAmount = B.transferFee > 0
? data.amount * (PERCENT_PRECISION + B.transferFee) / PERCENT_PRECISION
: data.amount;
} else {
transferInAmount = data.amount;
/// Decrease input amount for calculations
data.inAmount = A.transferFee > 0
? data.amount * (PERCENT_PRECISION - A.transferFee) / PERCENT_PRECISION
: data.amount;
}
```
```solidity=272
if (data.isExactOut) {
/// Increase input amount by inbound token fee
transferInAmount = A.transferFee > 0
? data.inAmount * (PERCENT_PRECISION + A.transferFee) / PERCENT_PRECISION
: data.inAmount;
require(data.inAmount <= data.inAmountMax, "Input amount is higher than maximum");
} else {
require(data.outAmount >= data.outAmountMin, "Output amount is lower than minimum");
}
```
```solidity=354
if (C.transferFee > 0) {
/// Increasing the output amount offsets the loss from the fee
data.outAmount = data.outAmount * (PERCENT_PRECISION + C.transferFee) / PERCENT_PRECISION;
}
```
for deflationary reflective tokens (which are quite popular), it is not possible to set a clear transferFee, as it varies for each transfer and is generally unpredictable. Has this been examined in the context of transfer-fee tokens? <br /> Firstly, it is essential to consider that the PERCENT_PRECISION might differ among various tokens, for instance, a conceivable token could impose a fee as minute as 0.000000000000000000000001%. <br /> Secondly, a significant issue arises, as the equation x * (1+f) * (1-f) does not equal x; if a token deducts a fee of f% from the increased value of x*(1+f), the final amount sent to the pool would be insufficient for the swap; to address this, the following equation must be solved: y * (DENOMINATOR-f) / DENOMINATOR = x; y = x * DENOMINATOR / (DENOMINATOR-f); it is crucial to note that this is not equivalent to x * (DENOMINATOR+f) / DENOMINATOR, although the values are closely related.
##### Status.
NEW
______
#### CRITICAL-5.
At
- NarfexExchangerRouter3.sol:131,148-149
```solidity=131
uint uintValue = oracle.getPrice(_token) * uint(_amount) / USDC_PRECISION;
```
```solidity=148
uint priceA = A.addr == address(USDC) ? USDC_PRECISION : A.price;
uint priceB = B.addr == address(USDC) ? USDC_PRECISION : B.price;
```
1 IDR = 0.000068 USDC = 6.8 * 1e-5 <br /> If their currency devalues by another 100 times, the oracle would return zero. <br /> It currently returns a price=68, although the exchange rate is 1.00 Indonesian Rupiah = 0.000067823873 US Dollars. <br /> We are already losing 1% in precision, which, in my opinion, is CRITICAL. <br /> The proposal is actually quite straightforward: set the price in the oracle not for 1 IDR but for 10e18 IDR, and then getAmountInUSDC = amount * getPrice / 10e18.
##### Status.
NEW
______
#### CRITICAL-6.
At
- NarfexExchangerRouter3.sol:472-514
```solidity=472
function _swap(
SwapData memory data
) internal nonReentrant
{
require(data.path.length > 1, "Path length must be at least 2 addresses");
uint lastIndex = data.path.length - 1;
Token memory A; /// First token
Token memory B; /// Last token
{
/// Get the oracle data for the first and last tokens
address[] memory sideTokens = new address[](2);
sideTokens[0] = data.path[0];
sideTokens[1] = data.path[lastIndex];
INarfexOracle.TokenData[] memory tokensData = oracle.getTokensData(sideTokens, true);
A = _assignTokenData(sideTokens[0], tokensData[0]);
B = _assignTokenData(sideTokens[1], tokensData[1]);
}
require(A.addr != B.addr, "Can't swap the same tokens");
if (A.isFiat && B.isFiat)
{ /// If swap between fiats
ExchangeData memory exchange = _getExchangeValues(A, B, data.amount, data.isExactOut);
_swapFiats(data.from, data.to, A, B, exchange);
return;
}
if (!A.isFiat && !B.isFiat)
{ /// Swap on DEX only
_swapOnlyDEX(data, A, B);
return;
}
if ((A.isFiat && B.addr == address(USDC))
|| (B.isFiat && A.addr == address(USDC)))
{ /// If swap between fiat and USDC in the pool
ExchangeData memory exchange = _getExchangeValues(A, B, data.amount, data.isExactOut);
_swapFiatAndUSDC(data, A, B, exchange, false);
return;
}
/// Swap with DEX and Fiats
data.path = _getDEXSubPath(data.path, A.isFiat);
_swapFiatWithDEX(data, A.isFiat ? A : B, A.isFiat ? B : A, A.isFiat);
}
```
I do think it's mandatory to put require for outAmountMin here. Be very strict. Do not believe that you expected pre-calculations will be correct. Do IERC20.balanceOf(to) call before and after the swap and ensure the difference is >= outAmountMin
##### Status.
NEW
______
#### CRITICAL-7.
At
- NarfexExchangerRouter3.sol:516-522
```solidity=516
/// @notice Set a new pool address
/// @param _newPoolAddress Another pool address
/// @param _decimals Pool token decimals
function setPool(address _newPoolAddress, uint8 _decimals) public onlyOwner {
pool = INarfexExchangerPool(_newPoolAddress);
USDC_PRECISION = 10**_decimals;
}
```
it's not "Pool token decimals" but "USDC decimals". I believe it should be set in the constructor and not changed later. Or if you change USDC decimals then also change USDC address.
##### Status.
NEW
______
#### MAJOR-1.
At
- NarfexLawyers.sol:145-152
```solidity=145
/// @notice Randomly returns the currently available lawyer
/// @return Active lawyer address
function getLawyer() public view returns(address) {
(address[] memory active, uint length) = getActiveLawyers();
if (length == 0) return address(0);
uint index = uint(keccak256(abi.encodePacked(block.timestamp, block.basefee))) % length;
return active[index];
}
```
this is not random! It may easily be manipulated via FlashBot attack
##### Status.
NEW
______
#### MAJOR-2.
At
- NarfexP2pRouter.sol:53-55
```solidity=53
path[0] = token;
path[1] = address(USDC);
path[2] = address(WETH);
```
it could be VERY inefficient, some tokens have big pool for token-WETH and small for token-USDC, such swap will lead to huge slippage. Consider removal of this function completely because you already have swap where path is explicitly passed.
##### Status.
NEW
______
#### MAJOR-3.
At
- NarfexP2pRouter.sol:79
```solidity=79
USDC.transferFrom(msg.sender, address(pool), _usdcAmount);
```
safeTransferFrom
##### Status.
NEW
______
#### MAJOR-4.
At
- NarfexP2pRouter.sol:83,94
```solidity=83
pool.increaseLimit(msg.sender, _fiatAddress, fiatAmount);
```
```solidity=94
pool.decreaseLimit(msg.sender, _fiatAddress, _fiatAmount);
```
??? i'm not sure. What if depositer will transfer out his tokens, limits will not be affected right? Unclear place.
##### Status.
NEW
______
#### MAJOR-5.
At
- NarfexP2pRouter.sol:99
```solidity=99
usdcAmount = usdcAmount * PERCENT_PRECISION / VALIDATOR_POOL_PERCENT;
```
?????? explain this formula in comments because it's not obvious why do you return bigger usdc amount. In fact it leads to mis-pricing of fiats. time=1 1usd=100rub, deposit 1usd get 80rub, time=2 1usd=20rub, withdraw 80rub, where to get 4usd?
##### Status.
NEW
______
#### MAJOR-6.
At
- NarfexP2pRouter.sol:102
```solidity=102
USDC.transferFrom(address(pool), msg.sender, usdcAmount);
```
safeTransferFrom
##### Status.
NEW
______
#### MAJOR-7.
At
- NarfexKYC.sol:10
```solidity=10
mapping(address=>string) private _clients;
```
explain why do you use string here? why not IPFS link? Is it "Encrypted JSON encoded account personnel data"? What does it mean encrypted? Who can decrypt it? What if decryption key will leak?
##### Status.
NEW
______
#### MAJOR-8.
At
- NarfexP2pBuyFactory.sol:86
```solidity=86
function create(address _fiatAddress, uint16 _commission) public {
```
what if validator disagree with Oracle price?
##### Status.
NEW
______
#### MAJOR-9.
At
- NarfexExchangerPool.sol:77
```solidity=77
token.approve(router, 0); /// Remove allowance
```
safeApprove?? IERC20 interface attack??
##### Status.
NEW
______
#### MAJOR-10.
At
- NarfexExchangerPool.sol:105
```solidity=105
token.transfer(_msgSender(), _amount);
```
safeTransfer
##### Status.
NEW
______
#### MAJOR-11.
At
- NarfexExchangerPool.sol:114
```solidity=114
IERC20(_address).transfer(_msgSender(), _amount);
```
safeTransfer
##### Status.
NEW
______
#### MAJOR-12.
At
- NarfexExchangerPool.sol:154
```solidity=154
emit WithdrawFee(feeAmount);
```
wrong event argument, place this one line above
##### Status.
NEW
______
#### MAJOR-13.
At
- NarfexFiat.sol:19
```solidity=19
contract NarfexFiat is IERC20 {
```
what is the guarantee that 1 NarfexFiat token actually matches 1 real fiat? Consider adding explanations in the contract.
##### Status.
NEW
______
#### MAJOR-14.
At
- NarfexExchangerRouter3.sol:GLOBAL
there is no unit tests!
##### Status.
NEW
______
#### MAJOR-15.
At
- NarfexExchangerRouter3.sol:GLOBAL
Consider refactoring. In general, I advocate for writing as little code as possible; many areas can be replaced with OpenZeppelin implementations of AccessControl and EnumerableSet.
##### Status.
NEW
______
#### WARNING-1.
At
- NarfexLawyers.sol:12
```solidity=12
bool[7][24] schedule;
```
should it be more flexible (e.g. 10:15-17:45)? what timezone is it?
##### Status.
NEW
______
#### WARNING-2.
At
- NarfexLawyers.sol:55-59
```solidity=55
for (uint8 w; w < 7; w++) {
for (uint8 h; h < 24; h++) {
schedule[w][h] = true;
}
}
```
all hours are true by default, is it correct?
##### Status.
NEW
______
#### WARNING-3.
At
- NarfexLawyers.sol:76
```solidity=76
for (uint i; i < list.length - 1; i++) {
```
very gas expensive O(n), use EnumerableMap with O(1)
##### Status.
NEW
______
#### WARNING-4.
At
- NarfexFiatFactory.sol:60
```solidity=60
fiats[tokenSymbol] = fiat;
```
require(fiats[tokenSymbol]==address(0), "already exists") ???
##### Status.
NEW
______
#### WARNING-5.
At
- INarfexOracle.sol:30
```solidity=30
function getPrice(address _address) external view returns (uint);
```
explain return how many decimals how is it calculated
##### Status.
NEW
______
#### WARNING-6.
At
- PancakeLibrary.sol:52-74
```solidity=52
function pairFor(address tokenA, address tokenB) internal view returns (address pair) {
(address token0, address token1) = sortTokens(tokenA, tokenB);
/// ETH data
bytes memory factory = hex'5C69bEe701ef814a2B6a3EDD4B1652CB9cc5aA6f';
bytes memory initCodeHash = hex'96e8ac4277198ff8b6f785478aa9a39f403cb768dd02cbee326c3e7da348845f';
if (block.chainid == 56) { /// BSC
factory = hex'cA143Ce32Fe78f1f7019d7d551a6402fC5350c73';
initCodeHash = hex'00fb7f630766e6a796048ea87d01acd3068e8ff67d078148a3fa3f4a84f69bd5';
}
if (block.chainid == 97) { /// BSC testnet
factory = hex'B7926C0430Afb07AA7DEfDE6DA862aE0Bde767bc';
initCodeHash = hex'ecba335299a6693cb2ebc4782e74669b84290b6378ea3a3873c7231a8d7d1074';
}
if (block.chainid == 137
|| block.chainid == 42161
|| block.chainid == 80001) { /// Polygon or Arbitrum
factory = hex'c35DADB65012eC5796536bD9864eD8773aBc74C4';
initCodeHash = hex'e18a34eb0e04b04f7a0ac29a6e80748dca96319b42c54d679cb821dca90c6303';
}
if (block.chainid == 324) { /// zkSync Era
factory = hex'40be1cBa6C5B47cDF9da7f963B6F761F4C60627D';
initCodeHash = hex'95d5c05820d58f1c8cc736b47fe10a29ddcd2cf73a0d842e8537b9fe510fc618';
}
```
check
##### Status.
NEW
______
#### WARNING-7.
At
- NarfexP2pRouter.sol:33
```solidity=33
uint amountLimit,
```
unclear complex semantic, use separate functions for exactIn and exactOut
##### Status.
NEW
______
#### WARNING-8.
At
- NarfexP2pRouter.sol:34,50
```solidity=34
uint deadline) external payable ensure(deadline)
```
```solidity=50
function swapToETH(address to, address token, uint amount) external {
```
consider to put nonReentrant to external functions
##### Status.
NEW
______
#### WARNING-9.
At
- NarfexFiat.sol:36-59
```solidity=36
/**
* @dev Sets the values for {name} and {symbol}, initializes {decimals} with
* a default value of 18.
*
* To select a different value for {decimals}, use {_setupDecimals}.
*
* All three of these values are immutable: they can only be set once during
* construction.
*/
constructor(
string memory tokenName,
string memory tokenSymbol,
address factoryAddress
) {
_name = tokenName;
_symbol = tokenSymbol;
_factory = INarfexFiatFactory(factoryAddress);
uint8 currentDecimals = 6;
if (block.chainid == 56 || block.chainid == 97) {
currentDecimals = 18;
}
_decimals = currentDecimals;
}
```
wrong comment about decimals
##### Status.
NEW
______
#### WARNING-10.
At
- NarfexFiat.sol:54
```solidity=54
uint8 currentDecimals = 6;
```
it looks unsafe, some unclear magic numbers. Consider the usage of _factory.defaultDecimals or USDC.decimals() or somethings like this.
##### Status.
NEW
______
#### WARNING-11.
At
- NarfexFiat.sol:61
```solidity=61
modifier onlyOwner {
```
rename to onlyFullAccess
##### Status.
NEW
______
#### WARNING-12.
At
- NarfexFiat.sol:GLOBAL
ask some native speaker to check all comments :-)
##### Status.
NEW
______
#### WARNING-13.
At
- NarfexFiat.sol:260
```solidity=260
_balances[sender] = _balances[sender] - amount;
```
silent failure if sender does not have enough balance, consider adding a require with error message before
##### Status.
NEW
______
#### WARNING-14.
At
- NarfexFiat.sol:296
```solidity=296
_balances[account] = _balances[account] - amount;
```
silent failure if account does not have enough balance, add require with error message before
##### Status.
NEW
______
#### WARNING-15.
At
- NarfexExchangerRouter3.sol:37
```solidity=37
contract NarfexExchangerRouter3 is Ownable, ReentrancyGuard {
```
You may consider employing the approach used for via.protocol, which operates with dozens of bridges and DEXs, similarly to 1inch and Eywa. <br /> There is a router that accepts the method execute(Operation[] operations). <br /> struct Operation { Adapter; TokenIn; AmountIn; TokenOut; minAmountOut; }. <br /> Your router remains minimally simple. <br /> Subsequently, the logic for working with each adapter is delegated to separate smart contracts: AdapterNarfexFiatToUsdc, AdapterPancakeswapV2, AdapterPancakeswapV3, and Adapter1Inch. <br /> This results in a minimalistic router that merely executes a for loop through an array of operations, while the logic for interacting with each DeFi product (including Narfex fiat to USDC) is contained within separate adapters, where the code is concise, simply restructuring arguments for a specific DEX to ensure a unified interface for all.
##### Status.
NEW
______
#### WARNING-16.
At
- NarfexExchangerRouter3.sol:95
```solidity=95
if (block.chainid == 56 || block.chainid == 97) {
```
use USDC.decimals()
##### Status.
NEW
______
#### WARNING-17.
At
- NarfexExchangerRouter3.sol:128
```solidity=128
/// @return USDC amount
```
it's wrong in fact
##### Status.
NEW
______
#### WARNING-18.
At
- NarfexExchangerRouter3.sol:205
```solidity=205
INarfexFiat(B.addr).mintTo(data.to, exchange.outAmount);
```
usdcAmount is not assigned
##### Status.
NEW
______
#### WARNING-19.
At
- NarfexExchangerRouter3.sol:206
```solidity=206
} else {
```
require B.addr == address(USDC)
##### Status.
NEW
______
#### WARNING-20.
At
- NarfexExchangerRouter3.sol:236-244,250-255
```solidity=236
function _processSwapData(SwapData memory data) internal view {
if (data.isExactOut) {
data.amounts = PancakeLibrary.getAmountsIn(data.outAmount, data.path);
data.inAmount = data.amounts[0];
} else {
data.amounts = PancakeLibrary.getAmountsOut(data.inAmount, data.path);
data.outAmount = data.amounts[data.amounts.length - 1];
}
}
```
```solidity=250
function _swapOnlyDEX(
SwapData memory data,
Token memory A,
Token memory B
) private
{
```
Why are we interacting directly with PancakeSwap instead of using their Router2? It seems that if the swap chain includes multiple tokens with feeOnTransfer, there might be an issue; I would recommend testing this on a testnet; at the very least, it seems that we should use the actual received target token amount taken from the return value of IPancakePair(PancakeLibrary.pairFor(input, output)).swap.
##### Status.
NEW
______
#### WARNING-21.
At
- NarfexExchangerRouter3.sol:236
```solidity=236
function _processSwapData(SwapData memory data) internal view {
```
We could refactor and logically separate everything to reduce the overall code complexity; essentially, we should only have a module for swapping fiat <-> USDC; for everything else, we can utilize the already-written router from PancakeSwap, which would likely reduce the volume of your router by a factor of 3.
##### Status.
NEW
______
#### WARNING-22.
At
- NarfexExchangerRouter3.sol:236
```solidity=236
function _processSwapData(SwapData memory data) internal view {
```
Will we support PancakeSwap v3 pairs? Currently, some new tokens are being deployed exclusively on v3!
##### Status.
NEW
______
#### WARNING-23.
At
- NarfexExchangerRouter3.sol:286
```solidity=286
assert(WETH.transfer(firstPair, transferInAmount));
```
strictly following IERC20, you HAVE TO use safeTransfer
##### Status.
NEW
______
#### WARNING-24.
At
- NarfexExchangerRouter3.sol:289,299
```solidity=289
data.from.transfer(msg.value - transferInAmount);
```
```solidity=299
data.to.transfer(data.outAmount);
```
never use transfer - https://consensys-net.webpkgcache.com/doc/-/s/consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
##### Status.
NEW
______
#### WARNING-25.
At
- NarfexExchangerRouter3.sol:492-497,503-509
```solidity=492
if (A.isFiat && B.isFiat)
{ /// If swap between fiats
ExchangeData memory exchange = _getExchangeValues(A, B, data.amount, data.isExactOut);
_swapFiats(data.from, data.to, A, B, exchange);
return;
}
```
```solidity=503
if ((A.isFiat && B.addr == address(USDC))
|| (B.isFiat && A.addr == address(USDC)))
{ /// If swap between fiat and USDC in the pool
ExchangeData memory exchange = _getExchangeValues(A, B, data.amount, data.isExactOut);
_swapFiatAndUSDC(data, A, B, exchange, false);
return;
}
```
put require path.length==2
##### Status.
NEW
______
#### WARNING-26.
At
- NarfexExchangerRouter3.sol:541
```solidity=541
uint amountLimit,
```
unclear complex semantic, use separate functions for exactIn and exactOut
##### Status.
NEW
______
#### WARNING-27.
At
- NarfexExchangerRouter3.sol:537-543
```solidity=537
function swap(
address[] memory path,
bool isExactOut,
uint amount,
uint amountLimit,
uint deadline) public payable ensure(deadline)
{
```
add returns actual tokens amounts. This could be useful for the contract usage.
##### Status.
NEW
______
#### LOW-1.
At
- NarfexLawyers.sol:12
```solidity=12
bool[7][24] schedule;
```
I'm not sure is it as efficient as bit-shifts
##### Status.
NEW
______
#### LOW-2.
At
- NarfexLawyers.sol:15
```solidity=15
address public writer;
```
you can use OpenZeppelin's AccessControl
##### Status.
NEW
______
#### LOW-3.
At
- NarfexLawyers.sol:16-18
```solidity=16
mapping (address=>Lawyer) private _lawyers;
mapping (address=>bool) private _isLawyer;
address[] public list;
```
use EnumerableMap
##### Status.
NEW
______
#### LOW-4.
At
- NarfexLawyers.sol:19
```solidity=19
uint constant DAY = 86400;
```
use "1 days" included Solidity constant
##### Status.
NEW
______
#### LOW-5.
At
- NarfexLawyers.sol:102
```solidity=102
_lawyers[msg.sender].schedule = _schedule;
```
emit event
##### Status.
NEW
______
#### LOW-6.
At
- NarfexLawyers.sol:108
```solidity=108
_lawyers[msg.sender].isActive = _newState;
```
emit event
##### Status.
NEW
______
#### LOW-7.
At
- NarfexLawyers.sol:125-126
```solidity=125
uint8 weekDay = uint8((block.timestamp / DAY + 4) % 7);
uint8 hour = uint8((block.timestamp / 60 / 60) % 24);
```
conversion to uint8 is redundant
##### Status.
NEW
______
#### LOW-8.
At
- NarfexLawyers.sol:131
```solidity=131
/// @return Array of addresses
```
note that last elements of array can be address(0), explain return
##### Status.
NEW
______
#### LOW-9.
At
- NarfexFiatFactory.sol:17
```solidity=17
/// Mapping TokenSymbol=>Address
```
you could use Openzeppelin EnumerableMap here
##### Status.
NEW
______
#### LOW-10.
At
- NarfexFiatFactory.sol:20
```solidity=20
NarfexFiat[] public fiatsList;
```
maybe the owner should be able to remove fiats from this list
##### Status.
NEW
______
#### LOW-11.
At
- NarfexFiatFactory.sol:23
```solidity=23
event CreateFiat(string tokenName, string tokenSymbol, NarfexFiat tokenAddress);
```
indexed
##### Status.
NEW
______
#### LOW-12.
At
- NarfexFiatFactory.sol:47
```solidity=47
function isHaveFullAccess(address account) internal view returns (bool) {
```
rename to hasFullAccess
##### Status.
NEW
______
#### LOW-13.
At
- INarfexOracle.sol:9
```solidity=9
bool isCustomReward; // Use defalt referral percent on false
```
typo in word "default"
##### Status.
NEW
______
#### LOW-14.
At
- INarfexOracle.sol:10
```solidity=10
uint price; // USD price only for fiats
```
decimals unclear add comment or calculations example
##### Status.
NEW
______
#### LOW-15.
At
- INarfexOracle.sol:11
```solidity=11
uint reward; // Referral percent only for fiats
```
decimals unclear add comment or calculations example
##### Status.
NEW
______
#### LOW-16.
At
- INarfexOracle.sol:12
```solidity=12
int commission; // Commission percent. Can be lower than zero
```
decimals unclear add comment or calculations example
##### Status.
NEW
______
#### LOW-17.
At
- INarfexOracle.sol:13
```solidity=13
uint transferFee; // Token transfer fee with 1000 decimals precision (20 for NRFX is 2%)
```
maybe 10_000?
##### Status.
NEW
______
#### LOW-18.
At
- NarfexP2pRouter.sol:8
```solidity=8
uint constant VALIDATOR_POOL_PERCENT = 8000;
```
explain how it works
##### Status.
NEW
______
#### LOW-19.
At
- NarfexP2pRouter.sol:27
```solidity=27
/// @dev Fiat to crypto must be exchanged via USDT
```
USDC
##### Status.
NEW
______
#### LOW-20.
At
- NarfexP2pRouter.sol:69
```solidity=69
function deposit(uint _usdcAmount, address _fiatAddress) public nonReentrant returns(uint) {
```
what does it do? add natspec comments
##### Status.
NEW
______
#### LOW-21.
At
- NarfexP2pRouter.sol:88
```solidity=88
function withdraw(uint _fiatAmount, address _fiatAddress) public nonReentrant returns(uint) {
```
what does it do? add natspec comments
##### Status.
NEW
______
#### LOW-22.
At
- NarfexKYC.sol:11
```solidity=11
mapping(address=>bool) private _verificators;
```
bad english, rename to verifiers
##### Status.
NEW
______
#### LOW-23.
At
- NarfexKYC.sol:13
```solidity=13
address public writer;
```
you can use OpenZeppelin's AccessControl
##### Status.
NEW
______
#### LOW-24.
At
- NarfexKYC.sol:56
```solidity=56
emit Verify(_account);
```
add _data to event
##### Status.
NEW
______
#### LOW-25.
At
- NarfexKYC.sol:59
```solidity=59
/// @notice Clead account personnel data
```
typo "Clean"
##### Status.
NEW
______
#### LOW-26.
At
- NarfexKYC.sol:79
```solidity=79
/// @notice Mark account as verificator
```
rename to verifier everywhere
##### Status.
NEW
______
#### LOW-27.
At
- NarfexKYC.sol:104
```solidity=104
emit Unblacklisted(_account);
```
bad english, rename to "RemovedFromBlacklist"
##### Status.
NEW
______
#### LOW-28.
At
- NarfexKYC.sol:111
```solidity=111
return _blacklisted[_account];
```
should it be EnumerableSet to let fetch all blacklisted accounts?
##### Status.
NEW
______
#### LOW-29.
At
- NarfexKYC.sol:116
```solidity=116
/// @return Is can trade
```
bad english
##### Status.
NEW
______
#### LOW-30.
At
- NarfexP2pBuyOffer.sol:15
```solidity=15
uint32 createDate;
```
rename to timestamp
##### Status.
NEW
______
#### LOW-31.
At
- NarfexP2pBuyOffer.sol:304
```solidity=304
require(_amount <= getAvailableBalance(), "Not enouth free balance");
```
typo enough
##### Status.
NEW
______
#### LOW-32.
At
- NarfexP2pBuyOffer.sol:328
```solidity=328
/// @notice Creade a new trade
```
typo create
##### Status.
NEW
______
#### LOW-33.
At
- NarfexP2pBuyOffer.sol:330
```solidity=330
/// @param bankAccountId Choosed bank account index
```
typo chosen
##### Status.
NEW
______
#### LOW-34.
At
- NarfexP2pBuyOffer.sol:369
```solidity=369
/// @param bankAccountId Choosed bank account index
```
typo chosen
##### Status.
NEW
______
#### LOW-35.
At
- NarfexP2pBuyFactory.sol:29
```solidity=29
contract NarfexP2pBuyFactory is Ownable {
```
Buy and Sell factories are similar, consider refactoring
##### Status.
NEW
______
#### LOW-36.
At
- NarfexP2pBuyFactory.sol:31
```solidity=31
struct Offer { /// Offer getter structure
```
remove "getter"
##### Status.
NEW
______
#### LOW-37.
At
- NarfexP2pBuyFactory.sol:37-41
```solidity=37
uint commission;
uint totalCommission;
uint minTrade;
uint maxTrade;
uint tradesQuote;
```
unclear semantics consider documenting
##### Status.
NEW
______
#### LOW-38.
At
- NarfexP2pBuyFactory.sol:49
```solidity=49
uint public tradesLimit = 2; /// Trades count per one offer in one time
```
how is it used?
##### Status.
NEW
______
#### LOW-39.
At
- TestnetUSDC.sol:23-24
```solidity=23
_name = 'Circle USD';
_symbol = 'USDC';
```
add TEST
##### Status.
NEW
______
#### LOW-40.
At
- NarfexExchangerPool.sol:21
```solidity=21
contract NarfexExchangerPool is Ownable {
```
inherit from INarfexExchangerPool
##### Status.
NEW
______
#### LOW-41.
At
- NarfexExchangerPool.sol:28
```solidity=28
mapping(address => mapping(address => uint)) public validatorLimit;
```
explain mapping keys
##### Status.
NEW
______
#### LOW-42.
At
- NarfexExchangerPool.sol:29
```solidity=29
uint private feeAmount;
```
explain what this is in the comment
##### Status.
NEW
______
#### LOW-43.
At
- NarfexExchangerPool.sol:34
```solidity=34
event Withdraw(address recipient, uint amount);
```
indexed
##### Status.
NEW
______
#### LOW-44.
At
- NarfexExchangerPool.sol:36
```solidity=36
event IncreaseValidatorLimit(address indexed validator, address fiat, uint amount);
```
indexed
##### Status.
NEW
______
#### LOW-45.
At
- NarfexExchangerPool.sol:37
```solidity=37
event DecreaseValidatorLimit(address indexed validator, address fiat, uint amount);
```
indexed
##### Status.
NEW
______
#### LOW-46.
At
- NarfexExchangerPool.sol:119
```solidity=119
emit IncreaseValidatorLimit(_validator, _fiatAddress, _amount);
```
emit final amount
##### Status.
NEW
______
#### LOW-47.
At
- NarfexExchangerPool.sol:125
```solidity=125
emit DecreaseValidatorLimit(_validator, _fiatAddress, _amount);
```
emit final amount
##### Status.
NEW
______
#### LOW-48.
At
- NarfexExchangerPool.sol:137
```solidity=137
feeAmount += _amount;
```
emit event
##### Status.
NEW
______
#### LOW-49.
At
- NarfexExchangerPool.sol:143
```solidity=143
path[0] = address(token);
```
I would recommend also to add separate function to withdraw token without swap
##### Status.
NEW
______
#### LOW-50.
At
- NarfexExchangerPool.sol:151
```solidity=151
block.timestamp + 20 minutes
```
not need to add anything
##### Status.
NEW
______
#### LOW-51.
At
- NarfexFiat.sol:7
```solidity=7
interface INarfexFiatFactory {
```
split to another file
##### Status.
NEW
______
#### LOW-52.
At
- NarfexFiat.sol:19
```solidity=19
contract NarfexFiat is IERC20 {
```
inherit from INarfexFiat
##### Status.
NEW
______
#### LOW-53.
At
- NarfexFiat.sol:30-32
```solidity=30
string private _name;
string private _symbol;
uint8 private _decimals;
```
Consider the usage of the "immutable" declaration.
##### Status.
NEW
______
#### LOW-54.
At
- NarfexFiat.sol:34
```solidity=34
uint256 constant MAX_INT = type(uint).max;
```
explicit "private" declaration.
##### Status.
NEW
______
#### LOW-55.
At
- NarfexFiat.sol:67
```solidity=67
* @dev Returns the bep token owner.
```
remove "bep"
##### Status.
NEW
______
#### LOW-56.
At
- NarfexFiat.sol:80
```solidity=80
/// @notice Returns true if the specified address have full access to user funds management
```
"has" not "have"
##### Status.
NEW
______
#### LOW-57.
At
- NarfexFiat.sol:83
```solidity=83
function isHaveFullAccess(address account) internal view returns (bool) {
```
rename to hasFullAccess
##### Status.
NEW
______
#### LOW-58.
At
- NarfexFiat.sol:88,95,97,102,109,116,123
```solidity=88
* @dev Returns the token name.
```
```solidity=95
* @dev Returns the token decimals.
```
```solidity=97
function decimals() public view returns (uint8) {
```
```solidity=102
* @dev Returns the token symbol.
```
```solidity=109
* @dev See {BEP20-totalSupply}.
```
```solidity=116
* @dev See {BEP20-balanceOf}.
```
```solidity=123
* @dev See {BEP20-transfer}.
```
everywhere use "@inheritdoc IERC20" instead
##### Status.
NEW
______
#### LOW-59.
At
- NarfexFiat.sol:164
```solidity=164
* required by the EIP. See the note at the beginning of {BEP20};
```
remove all this BEP20 comments, just use @inheritdoc everywhere
##### Status.
NEW
______
#### LOW-60.
At
- NarfexFiat.sol:202
```solidity=202
return true;
```
remove the return value, there is no standard for this method. It's not required.
##### Status.
NEW
______
#### LOW-61.
At
- NarfexFiat.sol:219,200
```solidity=219
function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) {
```
```solidity=200
function increaseAllowance(address spender, uint256 addedValue) public returns (bool) {
```
add NatSpec notice and param
##### Status.
NEW
______
#### LOW-62.
At
- NarfexFiat.sol:225
```solidity=225
return true;
```
remove the return value, there is no standard for this method. It's not required.
##### Status.
NEW
______
#### LOW-63.
At
- NarfexFiat.sol:228
```solidity=228
/// @notice Mint amount of tokens to specified reciever address
```
typo "receiver"
##### Status.
NEW
______
#### LOW-64.
At
- NarfexFiat.sol:232
```solidity=232
/// @return Always true
```
remove the return value, there is no standard for this method. It's not required.
##### Status.
NEW
______
#### LOW-65.
At
- NarfexFiat.sol:257-258,275,294,319-320
```solidity=257
require(sender != address(0), "BEP20: transfer from the zero address");
require(recipient != address(0), "BEP20: transfer to the zero address");
```
```solidity=275
require(account != address(0), "BEP20: mint to the zero address");
```
```solidity=294
require(account != address(0), "BEP20: burn from the zero address");
```
```solidity=319
require(owner != address(0), "BEP20: approve from the zero address");
require(spender != address(0), "BEP20: approve to the zero address");
```
to be strict it's not always BEP20, consider renaming with "IERC20:" or "NarfexFiat:"
##### Status.
NEW
______
#### LOW-66.
At
- NarfexExchangerRouter3.sol:10-29
```solidity=10
interface INarfexFiat is IERC20 {
function burnFrom(address _address, uint _amount) external;
function mintTo(address _address, uint _amount) external;
}
interface INarfexExchangerPool {
function getFeeAmount() external view returns (uint);
function getBalance() external view returns (uint);
function approveRouter() external;
function increaseLimit(address _validator, address _fiatAddress, uint _amount) external;
function decreaseLimit(address _validator, address _fiatAddress, uint _amount) external;
function increaseFeeAmount(uint _amount) external;
function clearFeeAmount() external;
}
interface IWETH {
function deposit() external payable;
function transfer(address to, uint value) external returns (bool);
function withdraw(uint) external;
}
```
split to other files and reuse
##### Status.
NEW
______
#### LOW-67.
At
- NarfexExchangerRouter3.sol:37
```solidity=37
contract NarfexExchangerRouter3 is Ownable, ReentrancyGuard {
```
rename to NarfexExchangerRouter
##### Status.
NEW
______
#### LOW-68.
At
- NarfexExchangerRouter3.sol:40
```solidity=40
/// Structures for solving the problem of limiting the number of variables
```
The presence of triple slashes (///) instead of double slashes (//) outside of NatSpec comments is unconventional; it is recommended to follow generally accepted practice; in essence, the utilization of triple slashes (///) can be considered erroneous, as the Solidity compiler (solc) interprets them as NatSpec comments with the Doxygen @notice tag; it is peculiar that the compiler processes them without any warnings.
##### Status.
NEW
______
#### LOW-69.
At
- NarfexExchangerRouter3.sol:43
```solidity=43
uint rate;
```
add comment or example of calculations, what is the denominator of the rate? consider documenting USDC_PRECISION
##### Status.
NEW
______
#### LOW-70.
At
- NarfexExchangerRouter3.sol:53-56
```solidity=53
uint inAmount;
uint inAmountMax;
uint outAmount;
uint outAmountMin;
```
what is the difference with inAmount, consider documenting or rename to actualInAmount, actualOutAmount
##### Status.
NEW
______
#### LOW-71.
At
- NarfexExchangerRouter3.sol:65,67-68
```solidity=65
int commission;
```
```solidity=67
uint reward;
uint transferFee;
```
denominated by PRECISION or PERCENT_PRECISION?
##### Status.
NEW
______
#### LOW-72.
At
- NarfexExchangerRouter3.sol:77
```solidity=77
uint internal USDC_PRECISION = 10**6;
```
set in constructor
##### Status.
NEW
______
#### LOW-73.
At
- NarfexExchangerRouter3.sol:79
```solidity=79
uint constant MAX_INT = 2**256 - 1;
```
use type(uint).max
##### Status.
NEW
______
#### LOW-74.
At
- NarfexExchangerRouter3.sol:102
```solidity=102
modifier ensure(uint deadline) {
```
rename with ensureDeadline
##### Status.
NEW
______
#### LOW-75.
At
- NarfexExchangerRouter3.sol:107
```solidity=107
event SwapFiat(address indexed _account, address _fromToken, address _toToken, ExchangeData _exchange);
```
indexed _fromToken _toToken
##### Status.
NEW
______
#### LOW-76.
At
- NarfexExchangerRouter3.sol:108
```solidity=108
event SwapDEX(address indexed _account, address _fromToken, address _toToken, uint inAmount, uint outAmount);
```
indexed _fromToken _toToken
##### Status.
NEW
______
#### LOW-77.
At
- NarfexExchangerRouter3.sol:112
```solidity=112
assert(msg.sender == address(WETH));
```
use require with error message
##### Status.
NEW
______
#### LOW-78.
At
- NarfexExchangerRouter3.sol:246
```solidity=246
/// @notice Exchange only between crypto Сoins through liquidity pairs
```
wrong term. replace Coins with Tokens, https://zerion.io/blog/crypto-coins-vs-tokens-the-difference-explained/
##### Status.
NEW
______
#### LOW-79.
At
- NarfexExchangerRouter3.sol:284
```solidity=284
require(msg.value >= transferInAmount, "BNB is not sended");
```
typo in "sent"
##### Status.
NEW
______
#### LOW-80.
At
- NarfexExchangerRouter3.sol:332
```solidity=332
function _swapFiatWithDEX(
```
very long function consider refactoring
##### Status.
NEW
______
#### LOW-81.
At
- NarfexExchangerRouter3.sol:474
```solidity=474
) internal nonReentrant
```
I would prefer to put nonReentrant on external/public functions instead
##### Status.
NEW
______
#### LOW-82.
At
- NarfexExchangerRouter3.sol:519,526
```solidity=519
function setPool(address _newPoolAddress, uint8 _decimals) public onlyOwner {
```
```solidity=526
function setOracle(address _newOracleAddress) public onlyOwner {
```
emit event
##### Status.
NEW
______
#### LOW-83.
At
- NarfexExchangerRouter3.sol:551
```solidity=551
data.inAmountMax = isExactOut ? amountLimit : MAX_INT;
```
looks scary, consider adding a comment
##### Status.
NEW
Sign in with Wallet
Connect another wallet