# Security Audit of `Narfex-masterchef` contracts. ## Conclusion This audit was made by Auditor: Vladimir Smelov <vladimirfol@gmail.com>. Date: 2023-03-20 TODO ## Scope TODO ## Methodology 1. Blind audit. Comprehend 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 - MasterChef.sol:219 ```solidity=219 return !isEarlyWithdraw; ``` should it be just "isEarlyWithdraw" not "!isEarlyWithdraw" ##### Status. NEW ______ #### MAJOR-2. At - MasterChef.sol:226-229,215-220,204-209,185-198,291,269 ```solidity=226 function getUserPoolSize(address _pairAddress, address _user) public view returns (uint) { uint256 _pid = poolId[_pairAddress]; return userInfo[_pid][_user].amount; } ``` ```solidity=215 function getIsEarlyWithdraw(address _pairAddress, address _user) internal view returns (bool) { uint256 _pid = poolId[_pairAddress]; UserInfo storage user = userInfo[_pid][_user]; bool isEarlyWithdraw = block.timestamp - user.depositTimestamp < earlyHarvestCommissionInterval; return !isEarlyWithdraw; } ``` ```solidity=204 function getIsUserCanHarvest(address _pairAddress, address _user) internal view returns (bool) { uint256 _pid = poolId[_pairAddress]; UserInfo storage user = userInfo[_pid][_user]; bool isEarlyHarvest = block.timestamp - user.harvestTimestamp < harvestInterval; return !isEarlyHarvest; } ``` ```solidity=185 function getUserReward(address _pairAddress, address _user) public view returns (uint256) { uint256 _pid = poolId[_pairAddress]; PoolInfo storage pool = poolInfo[_pid]; UserInfo storage user = userInfo[_pid][_user]; uint256 accRewardPerShare = pool.accRewardPerShare; uint256 lpSupply = pool.pairToken.balanceOf(address(this)); if (block.number > pool.lastRewardBlock && lpSupply != 0) { uint256 blocks = block.number - pool.lastRewardBlock; uint256 totalReward = lastBalance * blocks - k * blocks**2 / 2; uint256 reward = totalReward * pool.allocPoint / totalAllocPoint; accRewardPerShare += reward * 1e12 / lpSupply; } return user.amount * accRewardPerShare / 1e12 - user.withdrawnReward + user.storedReward; } ``` ```solidity=291 function getPoolUserData(address _pairAddress, address _user) public view returns ( ``` ```solidity=269 uint256 _pid = poolId[_pairAddress]; ``` if there is no poolId for not-exist _pairAddress, then _pid=0, and the function will return amount of the user 0 pool (which has different LP token), consider adding _poolExists check. You can use it as a modifier like onlyExistPool(_pairAddress) and use it on every method with _pairAddress argument ##### Status. NEW ______ #### MAJOR-3. At - MasterChef.sol:409 ```solidity=409 function withdraw(address _pairAddress, uint256 _amount) public { ``` nonReentrant ##### Status. NEW ______ #### MAJOR-4. At - MasterChef.sol:430 ```solidity=430 function emergencyWithdraw(address _pairAddress) public { ``` nonReentrant ##### Status. NEW ______ #### MAJOR-5. At - MasterChef.sol:443 ```solidity=443 function harvest(address _pairAddress) public { ``` nonReentrant ##### Status. NEW ______ #### WARNING-1. At - MasterChef.sol:66 ```solidity=66 uint256 public lastBalance; ``` why we need it if we can use narfex.balanceOf(address(this)) ##### Status. NEW ______ #### WARNING-2. At - MasterChef.sol:157 ```solidity=157 if (_withUpdate) { ``` should it be required? ##### Status. NEW ______ #### WARNING-3. At - MasterChef.sol:173 ```solidity=173 if (_withUpdate) { ``` should it be required? otherwise miscalculation ##### Status. NEW ______ #### WARNING-4. At - MasterChef.sol:353,190 ```solidity=353 uint256 lpSupply = pool.pairToken.balanceOf(address(this)); ``` ```solidity=190 uint256 lpSupply = pool.pairToken.balanceOf(address(this)); ``` im not sure but what will happen if someone just transfer LP to the balance of this contract (not with deposit but just with erc20 transfer), will it ruin the calculations and lead to mistakes in reward sharing? ##### Status. NEW ______ #### WARNING-5. At - MasterChef.sol:436 ```solidity=436 user.amount = 0; ``` better to reset storage before external call ##### Status. NEW ______ #### LOW-1. At - MasterChef.sol:98 ```solidity=98 /// @notice Returns the soil fertility ``` what is "soil fertility" ##### Status. NEW ______ #### LOW-2. At - MasterChef.sol:110 ```solidity=110 rewardToken.safeTransfer(address(msg.sender), amount); ``` msg.sender is already an address no conversions needed ##### Status. NEW ______ #### LOW-3. At - MasterChef.sol:193 ```solidity=193 uint256 totalReward = lastBalance * blocks - k * blocks**2 / 2; ``` hmmm ##### Status. NEW ______ #### LOW-4. At - MasterChef.sol:194 ```solidity=194 uint256 reward = totalReward * pool.allocPoint / totalAllocPoint; ``` to be honest, it should check that totalAllocPoint!=0 ##### Status. NEW ______ #### LOW-5. At - MasterChef.sol:204 ```solidity=204 function getIsUserCanHarvest(address _pairAddress, address _user) internal view returns (bool) { ``` prefix with _ ##### Status. NEW ______ #### LOW-6. At - MasterChef.sol:215 ```solidity=215 function getIsEarlyWithdraw(address _pairAddress, address _user) internal view returns (bool) { ``` prefix with _ ##### Status. NEW ______ #### LOW-7. At - MasterChef.sol:226 ```solidity=226 function getUserPoolSize(address _pairAddress, address _user) public view returns (uint) { ``` better to rename to getUserPoolAmount ##### Status. NEW ______ #### LOW-8. At - MasterChef.sol:421 ```solidity=421 pool.pairToken.safeTransfer(address(msg.sender), _amount); ``` msg.sender is already an address no conversions needed ##### Status. NEW ______ #### LOW-9. At - MasterChef.sol:434 ```solidity=434 pool.pairToken.safeTransfer(address(msg.sender), user.amount); ``` msg.sender is already an address no conversions needed ##### Status. NEW ______ #### LOW-10. At - MasterChef.sol:462 ```solidity=462 function rewardTransfer(UserInfo storage user, uint256 _amount, bool isWithdraw, uint256 _pid) internal { ``` prefix with _ ##### Status. NEW