# 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
Sign in with Wallet
Connect another wallet