# Security Audit of `SpotV1` contracts.
## Conclusion
This audit was made by
Auditor: TODO.
Date: 2023-03-24
## Scope
https://testnet.bscscan.com/address/0x36AD17070546CAB7BcCFecbe1e8DBC45DE933191
- IRDNRegistry.sol
- RDNRegistry.sol
- WithdrawAnyERC20Token.sol
https://testnet.bscscan.com/address/0xBC526044b63b0aEe97F66195B956aA932C6776e9
- ERC20Spot.sol
- SafeUniswapV2Router.sol
- SpotV1.sol
- IRDNRegistry.sol
- RDNOwnable.sol
https://testnet.bscscan.com/address/0x81953203B4DDa69eF89d4a2370B5DA7E782a6Fc6
- ISpotV1.sol
- SpotFactoryV1.sol
- IRDNRegistry.sol
md5 hashes:
- 557e42e5e9eee64eab9ab17c5ccad488 contracts/ERC20Spot.sol
- d9f4aa3a24ec1985b279e625e7f2525e contracts/IRDNRegistry.sol
- b291a12383ca9ed97198831ee0b33803 contracts/ISpotV1.sol
- 8fb5c78bf695e5f0b83aa771d4a5f9ea contracts/RDNOwnable.sol
- baff4823cf421cdb2a4bf02291a72f62 contracts/RDNRegistry.sol
- fda3b5335bab4284a0378c54118ac48d contracts/SafeUniswapV2Router.sol
- c6f88a1e59d65fe467cd08244101049c contracts/SpotFactoryV1.sol
- 098ea1a4a7ecf95f32875716d46289ce contracts/SpotV1.sol
- 22bad6e816918f504a649cd45228888c contracts/WithdrawAnyERC20Token.sol
## 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
#### MAJOR-1.
At
- SafeUniswapV2Router.sol:50
```solidity=50
return router.addLiquidity(tokenA, tokenB, amountA, amountB, 0, 0, to, deadline);
```
- SafeUniswapV2Router.sol:76
```solidity=76
(amountA, amountB) = router.removeLiquidity(tokenA, tokenB, balance, 0, 0, to, deadline);
```
minAmountOut=0 sandwich transaction / MEV Flashbots attacks are possible
##### Status.
NEW
______
#### MAJOR-2.
At
- SpotFactoryV1.sol:63
```solidity=63
IERC20(swap0.path[0]).safeTransferFrom(msg.sender, address(this), amount);
```
some deflationary or fee-on-transfer tokens may deliver you different amount of tokens, consider using balanceOf check to ensure how much tokens did you receive after transferFrom
##### Status.
NEW
______
#### MAJOR-3.
At
- SpotFactoryV1.sol:64
```solidity=64
IERC20(swap0.path[0]).approve(spot, amount);
```
use safeApprove
##### Status.
NEW
______
#### MAJOR-4.
At
- SpotV1.sol:82-89
```solidity=82
function deposit(
uint amount,
Swap memory swap0,
Swap memory swap1,
Swap memory swapReward0,
Swap memory swapReward1,
uint deadline
) public payable {
```
- SpotV1.sol:129-136
```solidity=129
function withdraw(
uint amountToBurn,
Swap memory swap0,
Swap memory swap1,
Swap memory swapReward0,
Swap memory swapReward1,
uint deadline
) public onlyRDNOwner(msg.sender) {
```
- SpotV1.sol:172-174
```solidity=172
function callAny(address payable _addr, bytes memory _data) public payable onlyRDNOwner(msg.sender) returns(bool success, bytes memory data){
(success, data) = _addr.call{value: msg.value}(_data);
}
```
- SpotV1.sol:176-180
```solidity=176
function restake(
Swap memory swapReward0,
Swap memory swapReward1,
uint deadline
) public {
```
use nonReentrant modifier to protect fron reentrancy attacks
##### Status.
NEW
______
#### MAJOR-5.
At
- SpotV1.sol:108
```solidity=108
IERC20(swap0.path[0]).safeTransferFrom(msg.sender, address(this), amount);
```
some deflationary or fee-on-transfer tokens may deliver you different amount of tokens, consider using balanceOf check to ensure how much tokens did you receive after transferFrom, otherwise your code will not work with such tokens because receiving amount < transferred
##### Status.
NEW
______
#### MAJOR-6.
At
- SpotV1.sol:287-288
```solidity=287
0,
0,
```
- SpotV1.sol:310-311
```solidity=310
0,
0,
```
no minAmountOut sandwich attacks / MVE flashbots possible
##### Status.
NEW
______
#### MAJOR-7.
At
- RDNOwnable.sol:22
```solidity=22
require(IRDNRegistry(registry).getUserIdByAddress(_userAddress) == ownerId, "RDNOwnable: access denied");
```
getUserIdByAddress returns 0 for non existent _userAddress, in case if ownerId=0, any non-existent user may pass this check. I'm not sure how serious is it. Probably you will never set ownerId=0. Maybe it makes sense to add explicit require that ownerId!=0. And add explaining comment to the code. Maybe use isRegisteredByAddress check.
##### Status.
NEW
______
#### WARNING-1.
At
- SpotFactoryV1.sol:13-17
```solidity=13
// todo
// ownerid shoud be active / RDNConnected
// tariffs / subscription rules
// onlyRDN
// BNB create and deposit
```
- SpotV1.sol:239
```solidity=239
// todo: return remained
```
- SpotV1.sol:268
```solidity=268
// todo: return remained
```
resolve todo
##### Status.
NEW
______
#### WARNING-2.
At
- WithdrawAnyERC20Token.sol:18
```solidity=18
if (_isDefaultAdminRole) {
```
i believe it should always happen otherwise there will be no admin
##### Status.
NEW
______
#### WARNING-3.
At
- WithdrawAnyERC20Token.sol:25
```solidity=25
IERC20(_token).transfer(_target, _amount);
```
use safeTransfer
##### Status.
NEW
______
#### WARNING-4.
At
- SafeUniswapV2Router.sol:12-22
```solidity=12
function safeSwapExactTokensForTokens(
IUniswapV2Router router,
uint256 amountIn,
uint256 amountOutMin,
address[] memory path,
address to,
uint256 deadline
) internal returns (uint256[] memory amounts) {
if (path[0] != path[path.length - 1])
amounts = router.swapExactTokensForTokens(amountIn, amountOutMin, path, to, deadline);
}
```
you may want to use swapExactTokensForTokensSupportingFeeOnTransferTokens https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L339
##### Status.
NEW
______
#### WARNING-5.
At
- SafeUniswapV2Router.sol:24
```solidity=24
function addAllLiquidity(
```
please note in the function comment that it may not spend all of your tokens balance just to not misuse it
##### Status.
NEW
______
#### WARNING-6.
At
- SpotV1.sol:17-30
```solidity=17
// todo
// return remained !!!
// swap fees support !!!
// safeERC20 !!!
// BNB support !!!
// restake rules
// transfers logs to userId
// informative name and symbol
// owner should be active
// tariffs / subscription rules
// deposit / withdraw without restake ??
// APR / APY
// StableSwap support
// !!!!!! init functions permissions, close reInit
```
resolve todos
##### Status.
NEW
______
#### WARNING-7.
At
- SpotV1.sol:42
```solidity=42
address public pool;
```
there is a lot of logic about interacting with IMasterChef2, but in the example contract there is no pool set so I cannot check interactions - https://testnet.bscscan.com/address/0xBC526044b63b0aEe97F66195B956aA932C6776e9#readContract pool==address(0) probably this one is meant to be used https://bscscan.com/address/0xa5f8C5Dbd5F286960b9d90548680aE5ebFf07652#code
##### Status.
NEW
______
#### WARNING-8.
At
- SpotV1.sol:104
```solidity=104
if (msg.value > 0) {
```
require path[0] == wETH?
##### Status.
NEW
______
#### WARNING-9.
At
- SpotV1.sol:105
```solidity=105
amount = msg.value;
```
require amount == msg.value?
##### Status.
NEW
______
#### WARNING-10.
At
- SpotV1.sol:121
```solidity=121
amountToMint = ((afterDepositBalance - beforeDepositBalance) * beforeDepositSupply) / beforeDepositBalance;
```
- SpotV1.sol:152
```solidity=152
uint amountToWithdraw = ((amountToBurn * beforeWithdrawBalance)) / beforeWithdrawSupply;
```
tiny rounding error may possible what may lead to tokens dust aggregation
##### Status.
NEW
______
#### WARNING-11.
At
- SpotV1.sol:282-291
```solidity=282
IUniswapV2Router(_router).addLiquidity(
token0,
token1,
amountIn0,
amountIn1,
0,
0,
address(this),
deadline
);
```
unused tokens remains on the contract balance, check https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router01.sol#L58 it transfers only amount needed for even addLiquidity ensure _returnRemainder is always called
##### Status.
NEW
______
#### WARNING-12.
At
- SpotV1.sol:336
```solidity=336
IUniswapV2Router(router).swapExactTokensForTokensSupportingFeeOnTransferTokens(
```
do you want to support Native currency?
##### Status.
NEW
______
#### WARNING-13.
At
- SpotV1.sol:360
```solidity=360
if (tokens[i] == address(0)) continue;
```
should we process native eth in that case?
##### Status.
NEW
______
#### WARNING-14.
At
- RDNOwnable.sol:14
```solidity=14
ownerId = _ownerId;
```
ownerId is unchangeable, maybe you will want to add a transferOwnership method (then not forget to change Factory mapping)
##### Status.
NEW
______
#### WARNING-15.
At
- ERC20Spot.sol:10
```solidity=10
mapping(uint => uint) private _balances;
```
it's not clear why mapping is from userId=>amount not address=>amount as usual. What is the overall business rationale behind this userId uint based mechanism? Can you write explaining comment?
##### Status.
NEW
______
#### WARNING-16.
At
- ERC20Spot.sol:11
```solidity=11
mapping(address => mapping(address => uint)) private _allowances;
```
allowance is given to address not userId, ensure there is no misusing possible
##### Status.
NEW
______
#### WARNING-17.
At
- ERC20Spot.sol:26
```solidity=26
uint userId = IRDNRegistry(registry).getUserIdByAddress(_account);
```
be careful with non existent user
##### Status.
NEW
______
#### WARNING-18.
At
- ERC20Spot.sol:50-64
```solidity=50
function transfer(address to, uint amount) external returns (bool) {
return false;
}
function approve(address spender, uint amount) external returns (bool) {
return false;
}
function transferFrom(
address from,
address to,
uint amount
) external returns (bool) {
return false;
}
```
not all DeFi projects correctly implement erc20 interface call and check return value after the call, so the may think that transfer did success because it did not fail, consider add a revert inside
##### Status.
NEW
______
#### WARNING-19.
At
- ERC20Spot.sol:76
```solidity=76
}
```
- ERC20Spot.sol:91
```solidity=91
}
```
external DeFi tool may wrongly estimate account balance. Transfer is emitted only on mint/burn however in RDNRegistry user may transfer the userId ownership to another wallet, no Transfer event will be emited in this case and some DeFi tools will still think that the previous wallet has this balance, but its not.
##### Status.
NEW
______
#### WARNING-20.
At
- RDNRegistry.sol:120
```solidity=120
users[_userId].userAddress = _newAddress;
```
emit event, it's important state change you may want to catch it on backend/frontend
##### Status.
NEW
______
#### WARNING-21.
At
- RDNRegistry.sol:138
```solidity=138
changeAddressAccess[_userId][_grantedAddress] = false;
```
changeAddressAddresses is not cleared, i'm not sure how serious is it, maybe use EnumerableSet for changeAddressAddresses for cheap deletion
##### Status.
NEW
______
#### WARNING-22.
At
- RDNRegistry.sol:146
```solidity=146
function levelUp(uint _userId, uint _level) public onlyRole(LEVELUP_ROLE) onlyValidUser(_userId) {
```
role system is account based, it may happen that account userId will change but the permission remain the same, is it OK?
##### Status.
NEW
______
#### WARNING-23.
At
- RDNRegistry.sol:180
```solidity=180
function grantCompleteAdmin(address _admin) public {
```
- RDNRegistry.sol:192
```solidity=192
function revokeCompleteAdmin(address _admin) public {
```
place onlyRole(DEFAULT_ADMIN_ROLE) modifier and use internal _grantRole to avoid multiple role checks and to state in the declaration that the method is for admin only
##### Status.
NEW
______
#### WARNING-24.
At
- RDNRegistry.sol:218
```solidity=218
function _addUser(address _userAddress, uint _parentId) private returns(uint) {
```
any parent may be set by the user, I don't know which mechanism will you use but you could add signature backend verification to prevent setting wrong parent
##### Status.
NEW
______
#### WARNING-25.
At
- RDNRegistry.sol:290-312
```solidity=290
function isActive(uint _userId) public view returns(bool) {
return (users[_userId].activeUntill > block.timestamp);
}
function getParentId(uint _userId) public view returns(uint) {
return users[_userId].parentId;
}
function getLevel(uint _userId) public view returns(uint) {
return users[_userId].level;
}
function getTariff(uint _userId) public view returns(uint) {
return users[_userId].tariff;
}
function getActiveUntill(uint _userId) public view returns(uint) {
return users[_userId].activeUntill;
}
function getUserAddress(uint _userId) public view returns(address) {
return users[_userId].userAddress;
}
```
- RDNRegistry.sol:318-320
```solidity=318
function getUser(uint _userId) public view returns(User memory) {
return users[_userId];
}
```
maybe use isValidUser or isRegisteredByAddress
##### Status.
NEW
______
#### WARNING-26.
At
- RDNRegistry.sol:322-324
```solidity=322
function getUserIdByAddress(address _userAddress) public view returns(uint) {
return userId[_userAddress];
}
```
it returns 0 if user not exist. It may lead to misuse the info for userId=0. Place isRegistered or is isValidUser modifier
##### Status.
NEW
______
#### WARNING-27.
At
- RDNRegistry.sol:343
```solidity=343
function isHasChangeAddressAccess(uint _userId, address _grantedAddress) public view returns(bool) {
```
check if _userId is valid, otherwise isHasChangeAddressAccess(666, address(0))==true
##### Status.
NEW
______
#### LOW-1.
At
- ERC20Spot.sol:6
```solidity=6
```
- ERC20Spot.sol:21
```solidity=21
```
- ERC20Spot.sol:24
```solidity=24
```
- RDNRegistry.sol:350-351
```solidity=350
```
remove blank line, follow style guide - https://docs.soliditylang.org/en/v0.8.17/style-guide.html
##### Status.
NEW
______
#### LOW-2.
At
- RDNOwnable.sol:11
```solidity=11
function initRDNOwnable(address _registry, uint _ownerId) internal {
```
- ERC20Spot.sol:16
```solidity=16
function initERC20Spot(address _registry, uint _ownerId) internal {
```
prefix internal method with _
##### Status.
NEW
______
#### LOW-3.
At
- RDNOwnable.sol:7
```solidity=7
contract RDNOwnable {
```
- ERC20Spot.sol:8
```solidity=8
contract ERC20Spot is IERC20, RDNOwnable {
```
put the "abstract" declaration for the contract which will not be deployed but just used as a parent contract
##### Status.
NEW
______
#### LOW-4.
At
- SpotV1.sol:94
```solidity=94
if (IMasterChef2(pool).pendingCake(poolIndex, address(this)) > 0) {
```
- SpotV1.sol:141
```solidity=141
if (IMasterChef2(pool).pendingCake(poolIndex, address(this)) > 0) {
```
use _pool, _poolIndex
##### Status.
NEW
______
#### LOW-5.
At
- WithdrawAnyERC20Token.sol:24-26
```solidity=24
function withdrawAnyERC20Token(address _token, address _target, uint _amount) public onlyRole(WITHDRAWANY_ROLE) {
IERC20(_token).transfer(_target, _amount);
}
```
- SafeUniswapV2Router.sol:12
```solidity=12
function safeSwapExactTokensForTokens(
```
- SafeUniswapV2Router.sol:12
```solidity=12
function safeSwapExactTokensForTokens(
```
- SafeUniswapV2Router.sol:24
```solidity=24
function addAllLiquidity(
```
- SafeUniswapV2Router.sol:53
```solidity=53
function removeAllLiquidity(
```
this function is never used in the scope of this audit
##### Status.
NEW
______
#### LOW-6.
At
- WithdrawAnyERC20Token.sol:3
```solidity=3
// SPDX-License-Identifier: MIT
```
- SpotFactoryV1.sol:2
```solidity=2
// SPDX-License-Identifier: MIT
```
- SpotV1.sol:2
```solidity=2
// SPDX-License-Identifier: MIT
```
- ISpotV1.sol:2
```solidity=2
// SPDX-License-Identifier: MIT
```
- ERC20Spot.sol:2
```solidity=2
// SPDX-License-Identifier: MIT
```
place on top https://docs.soliditylang.org/en/v0.8.19/layout-of-source-files.html#spdx-license-identifier
##### Status.
NEW
______
#### LOW-7.
At
- WithdrawAnyERC20Token.sol:8-12
```solidity=8
// interface IWithdrawAnyERC20Token {
// function withdrawAnyERC20Token(address _token, address _target, uint _amount) external;
// }
```
remove
##### Status.
NEW
______
#### LOW-8.
At
- WithdrawAnyERC20Token.sol:15
```solidity=15
bytes32 public constant WITHDRAWANY_ROLE = keccak256("WITHDRAWANY_ROLE");
```
place underscore
##### Status.
NEW
______
#### LOW-9.
At
- WithdrawAnyERC20Token.sol:19
```solidity=19
_setupRole(DEFAULT_ADMIN_ROLE, _admin);
```
- WithdrawAnyERC20Token.sol:21
```solidity=21
_setupRole(WITHDRAWANY_ROLE, _admin);
```
deprecated use _grantRole
##### Status.
NEW
______
#### LOW-10.
At
- WithdrawAnyERC20Token.sol:24-26
```solidity=24
function withdrawAnyERC20Token(address _token, address _target, uint _amount) public onlyRole(WITHDRAWANY_ROLE) {
IERC20(_token).transfer(_target, _amount);
}
```
emit event to make transfers made by calling withdrawAnyERC20Token
##### Status.
NEW
______
#### LOW-11.
At
- WithdrawAnyERC20Token.sol:24-26
```solidity=24
function withdrawAnyERC20Token(address _token, address _target, uint _amount) public onlyRole(WITHDRAWANY_ROLE) {
IERC20(_token).transfer(_target, _amount);
}
```
you may also add withdrawAnyNative method
##### Status.
NEW
______
#### LOW-12.
At
- SafeUniswapV2Router.sol:1
```solidity=1
// SPDX-License-Identifier: BSD-3-Clause
```
SPDX license is different from other files, are you sure?
##### Status.
NEW
______
#### LOW-13.
At
- SafeUniswapV2Router.sol:2
```solidity=2
pragma solidity ^0.8.6;
```
maybe use "0.8.17" for consistency
##### Status.
NEW
______
#### LOW-14.
At
- SafeUniswapV2Router.sol:12-22
```solidity=12
function safeSwapExactTokensForTokens(
IUniswapV2Router router,
uint256 amountIn,
uint256 amountOutMin,
address[] memory path,
address to,
uint256 deadline
) internal returns (uint256[] memory amounts) {
if (path[0] != path[path.length - 1])
amounts = router.swapExactTokensForTokens(amountIn, amountOutMin, path, to, deadline);
}
```
what safe check this function makes? it's not clear
##### Status.
NEW
______
#### LOW-15.
At
- SafeUniswapV2Router.sol:20
```solidity=20
if (path[0] != path[path.length - 1])
```
check that path.length > 0
##### Status.
NEW
______
#### LOW-16.
At
- SafeUniswapV2Router.sol:21
```solidity=21
amounts = router.swapExactTokensForTokens(amountIn, amountOutMin, path, to, deadline);
```
use named parameters to avoid misplacing args, see https://docs.soliditylang.org/en/v0.8.19/control-structures.html#function-calls-with-named-parameters
##### Status.
NEW
______
#### LOW-17.
At
- SafeUniswapV2Router.sol:33-35
```solidity=33
uint256,
uint256,
uint256
```
place NatSpec description of return values
##### Status.
NEW
______
#### LOW-18.
At
- SafeUniswapV2Router.sol:39-42
```solidity=39
if (IERC20(tokenA).allowance(address(this), address(router)) > 0) {
IERC20(tokenA).safeApprove(address(router), 0);
}
IERC20(tokenA).safeApprove(address(router), amountA);
```
- SafeUniswapV2Router.sol:45-48
```solidity=45
if (IERC20(tokenB).allowance(address(this), address(router)) > 0) {
IERC20(tokenB).safeApprove(address(router), 0);
}
IERC20(tokenB).safeApprove(address(router), amountB);
```
- SafeUniswapV2Router.sol:71-74
```solidity=71
if (IERC20(pair).allowance(address(this), address(router)) > 0) {
IERC20(pair).safeApprove(address(router), 0);
}
IERC20(pair).safeApprove(address(router), balance);
```
what is the purpose of this? why do we need to set allowance to 0
##### Status.
NEW
______
#### LOW-19.
At
- SpotFactoryV1.sol:11
```solidity=11
contract SpotFactoryV1 {
```
what is the purpose of this contract? write detailed NatSpec docs
##### Status.
NEW
______
#### LOW-20.
At
- SpotFactoryV1.sol:21
```solidity=21
using SafeERC20 for IERC20;
```
place on the top of the contract code
##### Status.
NEW
______
#### LOW-21.
At
- SpotFactoryV1.sol:30
```solidity=30
mapping(uint => mapping(uint => address)) public spots;
```
what is the key in this mapping, add comment
##### Status.
NEW
______
#### LOW-22.
At
- SpotFactoryV1.sol:51-54
```solidity=51
ISpotV1.Swap memory swap0,
ISpotV1.Swap memory swap1,
ISpotV1.Swap memory swapReward0,
ISpotV1.Swap memory swapReward1,
```
unclear semantics, consider documenting in NatSpec what is swap0 and swap1
##### Status.
NEW
______
#### LOW-23.
At
- SpotFactoryV1.sol:61
```solidity=61
ISpotV1(spot).deposit{value: msg.value}(amount, swap0, swap1, swapReward0, swapReward1, deadline);
```
- SpotFactoryV1.sol:65
```solidity=65
ISpotV1(spot).deposit(amount, swap0, swap1, swapReward0, swapReward1, deadline);
```
- SpotFactoryV1.sol:78
```solidity=78
ISpotV1(spot).init(wrapper, pool, router, stakingToken, rewardToken, poolIndex, ownerId, registry, address(this));
```
use named parameters for readability and to avoid misplacing args, see https://docs.soliditylang.org/en/v0.8.19/control-structures.html#function-calls-with-named-parameters
##### Status.
NEW
______
#### LOW-24.
At
- SpotFactoryV1.sol:80
```solidity=80
// pools[ownerId].push(poolIndex);
```
remove this comment
##### Status.
NEW
______
#### LOW-25.
At
- SpotFactoryV1.sol:89
```solidity=89
uint poolsCount = IMasterChef2(pool).poolLength();
```
IMasterChef2 is not part of this audit scope, I cannot be sure this is a valid pools length
##### Status.
NEW
______
#### LOW-26.
At
- SpotFactoryV1.sol:98
```solidity=98
return spots[ownerId][poolIndex];
```
add a check that this spot exists
##### Status.
NEW
______
#### LOW-27.
At
- SpotV1.sol:13
```solidity=13
contract SpotV1 is ERC20Spot {
```
inherit ISpotV1
##### Status.
NEW
______
#### LOW-28.
At
- SpotV1.sol:33-35
```solidity=33
// interaction changes:
// wrapper in init
// deposit BNB, withdraw BNB
```
remove these comments
##### Status.
NEW
______
#### LOW-29.
At
- SpotV1.sol:37-40
```solidity=37
struct Swap {
address[] path;
uint outMin;
}
```
remove it because it's already a part of ISpotV1
##### Status.
NEW
______
#### LOW-30.
At
- SpotV1.sol:43
```solidity=43
address public router;
```
maybe rename as IMasterChef2 public masterChef2Pool; for clarity
##### Status.
NEW
______
#### LOW-31.
At
- SpotV1.sol:51
```solidity=51
address public wrapper;
```
maybe delcare it as "IWrapper public wBNB;"
##### Status.
NEW
______
#### LOW-32.
At
- SpotV1.sol:53
```solidity=53
constructor() {
```
why is it empty? note that this contract is intended to be used with proxy
##### Status.
NEW
______
#### LOW-33.
At
- SpotV1.sol:82-89
```solidity=82
function deposit(
uint amount,
Swap memory swap0,
Swap memory swap1,
Swap memory swapReward0,
Swap memory swapReward1,
uint deadline
) public payable {
```
- SpotV1.sol:129-136
```solidity=129
function withdraw(
uint amountToBurn,
Swap memory swap0,
Swap memory swap1,
Swap memory swapReward0,
Swap memory swapReward1,
uint deadline
) public onlyRDNOwner(msg.sender) {
```
unclear functions semantic consider documenting
##### Status.
NEW
______
#### LOW-34.
At
- SpotV1.sol:161
```solidity=161
require(sentRecipient, "transfer ETH to recipeint failed");
```
typo in word recipient (also not ETH but BNB)
##### Status.
NEW
______
#### LOW-35.
At
- SpotV1.sol:166
```solidity=166
//burn
```
comment not need
##### Status.
NEW
______
#### LOW-36.
At
- SpotV1.sol:214
```solidity=214
function _buyLiquidity(
```
what does this function supposed to do? why does it have 2 swap arguments?
##### Status.
NEW
______
#### LOW-37.
At
- SpotV1.sol:220
```solidity=220
require(swap0.path[0] == swap1.path[0], "start tokens should be equal");
```
explain. write comments
##### Status.
NEW
______
#### LOW-38.
At
- SpotV1.sol:323
```solidity=323
token.safeApprove(spender, 0);
```
why do we need this?
##### Status.
NEW
______
#### LOW-39.
At
- SpotV1.sol:334
```solidity=334
if (path[0] == path[path.length - 1]) return;
```
maybe fail? it's something really unexpected
##### Status.
NEW
______
#### LOW-40.
At
- SpotV1.sol:337
```solidity=337
// IUniswapV2Router(router).swapExactTokensForTokens(
```
remove comment
##### Status.
NEW
______
#### LOW-41.
At
- SpotV1.sol:366
```solidity=366
require(sentRecipient, "transfer ETH to recipeint failed");
```
typo in word recipient (also in fact it's not ETH it's BNB)
##### Status.
NEW
______
#### LOW-42.
At
- IRDNRegistry.sol:12
```solidity=12
uint activeUntill;
```
- IRDNRegistry.sol:40
```solidity=40
function getActiveUntill(uint _userId) external view returns(uint);
```
- IRDNRegistry.sol:48
```solidity=48
function setActiveUntill(uint _userId, uint _activeUntill) external;
```
typo in word "until"
##### Status.
NEW
______
#### LOW-43.
At
- RDNOwnable.sol:8-9
```solidity=8
address public registry;
uint public ownerId;
```
consider the usage of immutable declaration or add setter
##### Status.
NEW
______
#### LOW-44.
At
- RDNOwnable.sol:13
```solidity=13
require(IRDNRegistry(registry).isValidUser(_ownerId));
```
add revert message for clarity of failed transactions
##### Status.
NEW
______
#### LOW-45.
At
- RDNOwnable.sol:17
```solidity=17
// modifier RDNOnly(address _sender) {
```
remove unused code
##### Status.
NEW
______
#### LOW-46.
At
- ERC20Spot.sol:13-14
```solidity=13
string private _name;
string private _symbol;
```
use immutable declaration
##### Status.
NEW
______
#### LOW-47.
At
- ERC20Spot.sol:18
```solidity=18
_name = "SPOTv0 Share Token";
```
what does v0 mean? Consider renaming to v1 or explain what does v0 mean
##### Status.
NEW
______
#### LOW-48.
At
- ERC20Spot.sol:28
```solidity=28
}
```
maybe it's useful to have function balanceOf(uint _userId) as well
##### Status.
NEW
______
#### LOW-49.
At
- ERC20Spot.sol:31
```solidity=31
return _allowances[owner][spender];
```
this will always return 0, since approve is not implemented
##### Status.
NEW
______
#### LOW-50.
At
- ERC20Spot.sol:68
```solidity=68
require(account != address(0), "ERC20: mint to the zero address");
```
everywhere, in general string error messages are suboptimal, consider the usage of custom solidity errors, see - https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2839
##### Status.
NEW
______
#### LOW-51.
At
- RDNRegistry.sol:9
```solidity=9
// Parent 0 address restricted
```
make the comment more clear
##### Status.
NEW
______
#### LOW-52.
At
- RDNRegistry.sol:15-29
```solidity=15
bytes32 public constant SETLEVEL_ROLE = keccak256("SETLEVEL_ROLE");
// admin role for userlevel change (levelUp function)
bytes32 public constant LEVELUP_ROLE = keccak256("LEVELUP_ROLE");
// admin role for changing factors contract address
bytes32 public constant FACTORSADDRESS_ROLE = keccak256("FACTORSADDRESS_ROLE");
// admin role for RDNPOS contract
bytes32 public constant TARIFFUPDATE_ROLE = keccak256("TARIFFUPDATE_ROLE");
// admin role for RDNPOS contract
bytes32 public constant ACTIVEUNTILLUPDATE_ROLE = keccak256("ACTIVEUNTILLUPDATE_ROLE");
// admin role fore points rate updating
bytes32 public constant POINTSRATEUPDATE_ROLE = keccak256("POINTSRATEUPDATE_ROLE");
// admin role for RDNDistributors configuration
bytes32 public constant SETDISTRIBUTOR_ROLE = keccak256("SETDISTRIBUTOR_ROLE");
// admin role for adding custom users
bytes32 public constant ADDUSERBYADMIN_ROLE = keccak256("ADDUSERBYADMIN_ROLE");
```
Rename SETLEVEL_ROLE to SET_LEVEL_ROLE and update references accordingly for better readability.
##### Status.
NEW
______
#### LOW-53.
At
- RDNRegistry.sol:20
```solidity=20
// admin role for RDNPOS contract
```
- RDNRegistry.sol:22
```solidity=22
// admin role for RDNPOS contract
```
unclear comment
##### Status.
NEW
______
#### LOW-54.
At
- RDNRegistry.sol:32
```solidity=32
mapping (address => uint) public userId;
```
- RDNRegistry.sol:34
```solidity=34
User[] public users;
```
- RDNRegistry.sol:36-37
```solidity=36
uint public usersCount;
mapping(uint => uint[]) public children;
```
- RDNRegistry.sol:32
```solidity=32
mapping (address => uint) public userId;
```
- RDNRegistry.sol:40-41
```solidity=40
mapping(uint => mapping(address => bool)) public changeAddressAccess;
mapping(uint => address[]) public changeAddressAddresses;
```
Consider declaring variables like userId, users, and children as internal to avoid having duplicate getters.
##### Status.
NEW
______
#### LOW-55.
At
- RDNRegistry.sol:35-36
```solidity=35
// gas saving counter
uint public usersCount;
```
it's saves nothing, array length is places at the storage slot of the array pointer and cheap to SLOAD (the same cheap as sload usersCount), see e.g. https://mixbytes.io/blog/collisions-solidity-storage-layouts
##### Status.
NEW
______
#### LOW-56.
At
- RDNRegistry.sol:37
```solidity=37
mapping(uint => uint[]) public children;
```
maybe use EnumerableSet for cheap checks for "consist" in future in case if you will need it
##### Status.
NEW
______
#### LOW-57.
At
- RDNRegistry.sol:41
```solidity=41
mapping(uint => address[]) public changeAddressAddresses;
```
never used for any logic, consider removal
##### Status.
NEW
______
#### LOW-58.
At
- RDNRegistry.sol:44
```solidity=44
address public factorsAddress;
```
this variable is never used in the scope of this audit
##### Status.
NEW
______
#### LOW-59.
At
- RDNRegistry.sol:46
```solidity=46
// token => rate (rate is 1/USDprice for token, based in token.decimals)
```
the comment is unclear, what rate is it
##### Status.
NEW
______
#### LOW-60.
At
- RDNRegistry.sol:47
```solidity=47
mapping (address => uint) public pointsRate;
```
this is never used in the scope of this audit for any logic
##### Status.
NEW
______
#### LOW-61.
At
- RDNRegistry.sol:58
```solidity=58
// when users activeUntill updated
```
everywhere in project fix typo in word "until"
##### Status.
NEW
______
#### LOW-62.
At
- RDNRegistry.sol:71-72
```solidity=71
* @param _root: userAdrress for userId = 1
* @param _admin: default admin
```
typo in word address
##### Status.
NEW
______
#### LOW-63.
At
- RDNRegistry.sol:71
```solidity=71
* @param _root: userAdrress for userId = 1
```
NatSpec comments do not need ":"
##### Status.
NEW
______
#### LOW-64.
At
- RDNRegistry.sol:80
```solidity=80
//add root user (userId 1), referencing parantId=0.
```
typo in word parentId
##### Status.
NEW
______
#### LOW-65.
At
- RDNRegistry.sol:81
```solidity=81
User memory _rootUser = User(12, _root, 0, 7, block.timestamp + 36500 days, block.timestamp);
```
what does the magic value "12" and "7" mean?
##### Status.
NEW
______
#### LOW-66.
At
- RDNRegistry.sol:117
```solidity=117
emit UserAddressChanged(_userId, _newAddress, msg.sender, users[_userId].userAddress);
```
tiny gas optimisation is possible
##### Status.
NEW
______
#### LOW-67.
At
- RDNRegistry.sol:126-127
```solidity=126
function grantChangeAddressAccess(address _grantedAddress) public onlyRegisteredByAddress(msg.sender) {
uint _userId = userId[msg.sender];
```
gas optimization possible, userId sload twice (inside modifier and below)
##### Status.
NEW
______
#### LOW-68.
At
- RDNRegistry.sol:158
```solidity=158
factorsAddress = _factorsAddress;
```
maybe emit event here
##### Status.
NEW
______
#### LOW-69.
At
- RDNRegistry.sol:177
```solidity=177
distributors[_token] = _distributor;
```
emit event
##### Status.
NEW
______
#### LOW-70.
At
- RDNRegistry.sol:205
```solidity=205
* @notice sender must have 3 roles
```
list all required roles
##### Status.
NEW
______
#### LOW-71.
At
- RDNRegistry.sol:208
```solidity=208
(uint _userId) = _addUser(_userAddress, _parentId);
```
brackets are not needed
##### Status.
NEW
______
#### LOW-72.
At
- RDNRegistry.sol:211
```solidity=211
setLevel(_userId, _level);
```
maybe emit specific event to note that the user was added by admin
##### Status.
NEW
______
#### LOW-73.
At
- RDNRegistry.sol:222
```solidity=222
User memory _user = User(1, _userAddress, _parentId, 0, 0, block.timestamp);
```
better to use User({level: 1, userAddress: _userAddress, ...}) for readability and decreasing the chance to misuse an argument
##### Status.
NEW
______
#### LOW-74.
At
- RDNRegistry.sol:224-228
```solidity=224
usersCount += 1;
userId[_userAddress] = usersCount - 1;
children[_parentId].push(usersCount - 1);
emit UserAdded(usersCount - 1, _parentId, _userAddress);
return (usersCount - 1);
```
SLOAD(usersCount) called 5 times, it's inefficient, consider to put it to stack
##### Status.
NEW
______
#### LOW-75.
At
- RDNRegistry.sol:238
```solidity=238
require(isRegisteredByAddress(_userAddress), "user not registered");
```
gas optimisation is possible, instead of using this check as modifier and then get userId you can write getUserIdByAddressSafe reverting if user does not exist, and use it at the beginning of functions
##### Status.
NEW
______
#### LOW-76.
At
- RDNRegistry.sol:242
```solidity=242
modifier onlyRegistered(uint _userId) {
```
this modifier is never used in the scope of this audit
##### Status.
NEW
______
#### LOW-77.
At
- RDNRegistry.sol:247
```solidity=247
modifier onlyValidUser(uint _userId) {
```
it's not clear what does it mean "valid" what is the difference with onlyRegisteredByAddress, add comment to explain
##### Status.
NEW
______
#### LOW-78.
At
- RDNRegistry.sol:330
```solidity=330
function getChildren(uint _userId) public view returns(uint[] memory) {
```
check if _userId is valid
##### Status.
NEW
Sign in with Wallet
Connect another wallet