---
# System prepended metadata

title: Security Audit of `Narfex/Oracle` contracts.

---

# Security Audit of `Narfex/Oracle` contracts. 

## Conclusion

This audit was made by 
Auditor: Vladimir Smelov <vladimirfol@gmail.com>.
Date: 2023-02-06

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

At
- NarfexOracle.sol:4

```solidity=4
import "./PancakeLibrary.sol";
```

cannot compile the code since there is no such file in the repo


______

#### CRITICAL-2 - DISCUSSING.

At
- NarfexOracle.sol:63

```solidity=63
    function getPairRatio(address _token0, address _token1) internal view returns (uint) {
```

weak for price manipulations, consider the usage of uniswap v2 cumulative price oracle - https://docs.uniswap.org/contracts/v2/concepts/core-concepts/oracles

##### Client's comment

> Не знаю, как решить эту проблему

##### Reaudit

Вместо получения цены в конкретный момент в пулле, мы можем брать среднии цены последние 15 минут или за последний час.

Вот тут есть примерчик от самого юнисвопа - https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleOracleSimple.sol

______

#### CRITICAL-3 - DISCUSSING

At
- NarfexOracle.sol:86

```solidity=86
            : getDEXPrice(_address);
```

if backend goes offline, the oracle will give old prices. Consider returning the timestamp (or blocknumber) of the last price update, so the consumer will be able to understand if the prices are out-dated.

##### Client's comment

> Ну вот это по-любому надо как-то решить. Нужен надёжный источник, где можно взять цену


##### Reaudit

нужно возвращать вместе с ценой еще и метку времени последнего обновления цены

```
price, ts = getPrice()
require(now - ts <= 10 minutes)
```

______

#### MAJOR-1 - DISCUSSING

At
- NarfexOracle.sol:67

```solidity=67
        return pair.token0() == _token0
```

pancakeLibrary gives the price with fee, consider just using division of reserves

##### Clients comment

> В целом цена из pancakeLibrary для сторонних токенов в текущем Oracle используется только для массового получения цен на фронтенде. Роутер считает цену той же pancakeLibrary уже на месте, и цена с комиссией контракта LP-токена важна.

##### Reply

нужно подумать ...
а почему цена с комиссией важна? как вообще эти цены с оракула потом использоваться будут?
нужно еще принять во внимание что если например токен ВоваКойин внутри своего трансфера берет burningFee и 10% токенов просто сжигает, никакой ДЕКС в цене это не отобразит.

______

#### MAJOR-2.

At
- NarfexOracle.sol:76

```solidity=76
            : getPairRatio(_address, USDT);
```

not every token has a pair for Token-USDT, a lot of tokens have only Token-ETH pair. What should we do in this case? Should we take the price using the route Token-BNB BNB-USDT?

##### Client's comment
> При обмене в роутер посылается путь из токенов. Да, не обязательно есть пара к USDT. Но фронтенд тоже рассчитывает цену по пути через популярные токены, а цена к USDT отображается только в списке токенов, где отображаются балансы. Так что это не критично.


##### Reply

имеется ввиду что не нужно надеятся иметь возможность получать все цены с дексов от пары ShitCoin - USDT такие пары могут не существовать.

Можно проанализировать насколько таких токенов много.

Можно сделать в контракте возможность указывать address[] path для каждого токена по какому path смотреть его цену


______

#### MAJOR-3.

At
- NarfexOracle.sol:91,248,263

```solidity=91
    /// @return USD prices array with 18 digits of precision
```
```solidity=248
    /// @param _price Fiat price - unsigned number with 18 digits of precision
```
```solidity=263
    /// @param _prices Fiats prices array - unsigned numbers with 18 digits of precision
```

why you say 18 decimals? USDT.decimals()==6, USDC.decimals()==6, for them the price will be PancakeLibrary.getAmountOut(10**IERC20(_token0).decimals(), reserve0, reserve1)==10**6

##### Client's comment

> Комментарии забыл исправить, да. Важное назначение Оракула: передать именно соотношение нашего фиата к USDC в точностью 18 знаков. Т.е. это не отображает, сколько именно USDC , будет получено. Важен именно коэфициент фиат/USDC. А у фиата в каждой сети столько же знаков после запятой, сколько у USDC. Т.е. в сетях 59 и 97 это 18 знаков, а во всех остальных — 6.

###### Reply

Нужно очень аккуратным с этим быть))
можно тестов написать на разные decimals


______

#### MAJOR-4.

At
- NarfexOracle.sol:250

```solidity=250
    function updatePrice(address _address, uint _price) public canUpdate {
```

centralization power risk. Hacker will get the full control over the oracle if he will be in control of one backend server with a private key. Consider the usage of "voting" by several oracle nodes. Committing prices with signatures.

##### Status.
NEW


______

#### MAJOR-5.

At
- NarfexOracle.sol:282

```solidity=282
                delete fiats[i];
```

you have gaps in array, replace with the last one and use ".pop()"

##### Status.
NEW


______

#### WARNING-1.

At
- NarfexOracle.sol:9

```solidity=9
    function decimals() external view returns (uint8);
```

not all erc20 tokens have it

##### Status.
NEW


______

#### WARNING-2.

At
- NarfexOracle.sol:48

```solidity=48
    address public USDT; // Tether address in current network
```

it's here "Tether USDT", however on the call you told that we are gonna use "USDC", should we rename?

##### Status.
NEW


______

#### WARNING-3.

At
- NarfexOracle.sol:218

```solidity=218
        tokenData.price = !tokenData.isFiat && _skipCoinPrice
```

why do you have it for "!tokenData.isFiat"?

##### Status.
NEW


______

#### WARNING-4.

At
- NarfexOracle.sol:285

```solidity=285
        }
```

should it update the price?

##### Status.
NEW


______

#### LOW-1.

At
- NarfexOracle.sol:8

```solidity=8
interface IERC20 {
```

use import IERC20Metadata from OpenZeppelin

##### Status.
NEW


______

#### LOW-2.

At
- NarfexOracle.sol:23

```solidity=23
        bool isCustomReward; // Use defalt referral percent on false
```

typo in the word "default"

##### Status.
NEW


______

#### LOW-3.

At
- NarfexOracle.sol:25

```solidity=25
        uint reward; // Referral percent only for fiats
```

where is it used?

##### Status.
NEW


______

#### LOW-4.

At
- NarfexOracle.sol:25-27

```solidity=25
        uint reward; // Referral percent only for fiats
        int commission; // Commission percent with. Can be lower than zero
        uint transferFee; // Token transfer fee with 1000 decimals precision (20 for NRFX is 2%)
```

add postfix Numerator for clarity

##### Status.
NEW


______

#### LOW-5.

At
- NarfexOracle.sol:25-27

```solidity=25
        uint reward; // Referral percent only for fiats
        int commission; // Commission percent with. Can be lower than zero
        uint transferFee; // Token transfer fee with 1000 decimals precision (20 for NRFX is 2%)
```

should it be part of the oracle? Maybe split it to another contract keeping the oracle as simply as possible and logically isolated.

##### Status.
NEW


______

#### LOW-6.

At
- NarfexOracle.sol:39-40

```solidity=39
    address[] public fiats; // List of tracked fiat stablecoins
    address[] public coins; // List of crypto tokens with different commission
```

OpenZeppelin enumerableSet

##### Status.
NEW


______

#### LOW-7.

At
- NarfexOracle.sol:40

```solidity=40
    address[] public coins; // List of crypto tokens with different commission
```

rename to tokensWithCustomCommission

##### Status.
NEW


______

#### LOW-8.

At
- NarfexOracle.sol:44

```solidity=44
    int defaultCryptoCommission = 0; // Use as a commission if isCustomCommission = false for coins
```

wrong terminology https://www.bitstamp.net/learn/crypto-101/crypto-coins-vs-tokens/

##### Status.
NEW


______

#### LOW-9.

At
- NarfexOracle.sol:47

```solidity=47
    address public updater; // Updater account. Has rights for update prices
```

rephrase "to update"

##### Status.
NEW


______

#### LOW-10.

At
- NarfexOracle.sol:48

```solidity=48
    address public USDT; // Tether address in current network
```

"the current"

##### Status.
NEW


______

#### LOW-11.

At
- NarfexOracle.sol:48

```solidity=48
    address public USDT; // Tether address in current network
```

declare immutable

##### Status.
NEW


______

#### LOW-12.

At
- NarfexOracle.sol:50

```solidity=50
    event SetUpdater(address updaterAddress);
```

add events emitted on important settings set

##### Status.
NEW


______

#### LOW-13.

At
- NarfexOracle.sol:62,72

```solidity=62
    // Returns ratio
```
```solidity=72
    // Returns token USD price
```

use NatSpec

##### Status.
NEW


______

#### LOW-14.

At
- NarfexOracle.sol:95,108,174-179

```solidity=95
        for (uint i = 0; i < length; i++) {
```
```solidity=108
        for (uint i = 0; i < length; i++) {
```
```solidity=174
        for (uint i = 0; i < fiatsLength; i++) {
            responseTokens[i] = fiats[i];
        }
        for (uint i = 0; i < coinsLength; i++) {
            responseTokens[fiatsLength + i] = coins[i];
        }
```

use unchecked declaration to disable embedded safemath for i++

##### Status.
NEW


______

#### LOW-15.

At
- NarfexOracle.sol:157-160,164-166

```solidity=157
    /// @return Array of fiats addresses
    function getFiats() public view returns (address[] memory) {
        return fiats;
    }
```
```solidity=164
    function getCoins() public view returns (address[] memory) {
        return coins;
    }
```

fiats are declared as a public variable with default getter function. This is duplication. Consider removal.

##### Status.
NEW


______

#### LOW-16.

At
- NarfexOracle.sol:188

```solidity=188
    function getSettings() public view returns (
```

why do you need this function at all?

##### Status.
NEW


______

#### LOW-17.

At
- NarfexOracle.sol:197

```solidity=197
        for (uint i; i < length; i++) {
```

prevent the warning from slither, use "uint i = 0;:

##### Status.
NEW


______

#### LOW-18.

At
- NarfexOracle.sol:233

```solidity=233
        for (uint i; i < _tokens.length; i++) {
```

prevent the warning from slither, use "uint i = 0;:

##### Status.
NEW


______

#### LOW-19.

At
- NarfexOracle.sol:253

```solidity=253
            token.price = _price;
```

emit event?

##### Status.
NEW


______

#### LOW-20.

At
- NarfexOracle.sol:256

```solidity=256
            token.isFiat = true;
```

instead, it's safer to require(token.isFiat);

##### Status.
NEW


______

#### LOW-21.

At
- NarfexOracle.sol:276

```solidity=276
    function removeTokenFromFiats(address _address) public onlyOwner {
```

use openzeppelin EnumerableSet

##### Status.
NEW


______

#### LOW-22.

At
- NarfexOracle.sol:313-399

```solidity=313
    function updateDefaultSettings(
        int _fiatCommission,
        int _cryptoCommission,
        uint _reward
        ) public onlyOwner {
        defaultFiatCommission = _fiatCommission;
        defaultCryptoCommission = _cryptoCommission;
        defaultReward = _reward;
    }

    /// @notice Update tokens commissions
    /// @param tokensToCustom Array of tokens addresses which should stop using the default value
    /// @param tokensToDefault Array of tokens addresses which should start using the default value
    /// @param tokensChanged Array of tokens addresses that will receive changes
    /// @param newValues An array of commissions corresponding to an array of tokens
    /// @dev Only owner can use it
    function updateCommissions(
        address[] calldata tokensToCustom,
        address[] calldata tokensToDefault,
        address[] calldata tokensChanged,
        int[] calldata newValues
        ) public onlyOwner {
            require (tokensChanged.length == newValues.length, "Changed tokens length do not match values length");
            for (uint i = 0; i < tokensToCustom.length; i++) {
                Token storage token = tokens[tokensToCustom[i]];
                token.isCustomCommission = true;
                if (!token.isFiat) {
                    coins.push(tokensToCustom[i]);
                }
            }
            for (uint i = 0; i < tokensToDefault.length; i++) {
                Token storage token = tokens[tokensToCustom[i]];
                token.isCustomCommission = false;
                if (!token.isFiat) {
                    removeTokenFromCoins(tokensToDefault[i]);
                }
            }
            for (uint i = 0; i < tokensChanged.length; i++) {
                tokens[tokensToCustom[i]].commission = newValues[i];
            }
        }

    /// @notice Update default values and tokens commissions by one request
    /// @param _defaultFiatCommission Default fiat commission
    /// @param _defaultCryptoCommission Default coin commission
    /// @param _defaultReward Default referral reward percent
    /// @param tokensToCustom Array of tokens addresses which should stop using the default value
    /// @param tokensToDefault Array of tokens addresses which should start using the default value
    /// @param tokensChanged Array of tokens addresses that will receive changes
    /// @param newValues An array of commissions corresponding to an array of tokens
    /// @dev Only owner can use it
    function updateAllCommissions(
        int _defaultFiatCommission,
        int _defaultCryptoCommission,
        uint _defaultReward,
        address[] calldata tokensToCustom,
        address[] calldata tokensToDefault,
        address[] calldata tokensChanged,
        int[] calldata newValues
    ) public onlyOwner {
        updateCommissions(tokensToCustom, tokensToDefault, tokensChanged, newValues);
        updateDefaultSettings(_defaultFiatCommission, _defaultCryptoCommission, _defaultReward);
    }

    /// @notice Update referral rewards percents for many fiats
    /// @param tokensToCustom Array of tokens addresses which should stop using the default value
    /// @param tokensToDefault Array of tokens addresses which should start using the default value
    /// @param tokensChanged Array of tokens addresses that will receive changes
    /// @param newValues An array of percents corresponding to an array of tokens
    /// @dev Only owner can use it
    function updateReferralPercents(
        address[] calldata tokensToCustom,
        address[] calldata tokensToDefault,
        address[] calldata tokensChanged,
        uint[] calldata newValues
        ) public onlyOwner {
            require (tokensChanged.length == newValues.length, "Changed tokens length do not match values length");
            for (uint i = 0; i < tokensToCustom.length; i++) {
                tokens[tokensToCustom[i]].isCustomReward = true;
            }
            for (uint i = 0; i < tokensToDefault.length; i++) {
                tokens[tokensToCustom[i]].isCustomReward = false;
            }
            for (uint i = 0; i < tokensChanged.length; i++) {
                tokens[tokensToCustom[i]].reward = newValues[i];
            }
        }
```

it looks like it should not be part of the oracle

##### Status.
NEW


______

#### LOW-23.

At
- NarfexOracle.sol:317

```solidity=317
        ) public onlyOwner {
```

emit event

##### Status.
NEW


