EthereumCommonwealth / Auditing

Ethereum Commonwealth Security Department conducted over 400 security audits since 2018. Not even a single contract that we audited was hacked. You can access our audit reports in the ISSUES of this repo. We are accepting new audit requests.
https://audits.callisto.network/
GNU General Public License v3.0
131 stars 34 forks source link

NFTLootBox.com Audit of NFTLootbox.sol #460

Closed mydogmaxuk closed 3 years ago

mydogmaxuk commented 3 years ago

Audit request

Contract used for opening lootboxes and claiming rewards.

Source code

https://github.com/NFTLootBox/contracts/blob/master/contracts/NFTLootbox.sol

Disclosure policy

notify me privately in case of finding critical mistakes.

... provide your conditions for publishing the report or leave only standard disclosure policy link ...

Standard disclosure policy.

Contact information (optional)

Telegram @ChonkyBob Twitter @NFTLootbox

... Provide information about the media resources of the project you want us to audit (website/ twitter account/ reddit/ telegram channel/ etc.) ...

NFTLootBox.com t.me/NFTLootBoxChat

Platform

Eth

yuriy77k commented 3 years ago

The audit fee for NFTLootbox.sol file is 529 USDT (you may send it as ERC20 or TRC20 - let me know your choice). Please note, that this contract import other files (Context.sol, SafeMath.sol, Ownable.sol, IERC20.sol, IERC1155.sol, ReentrancyGuard.sol) which are excluded from the audit, and we will write about it in the final report. If you want to include those files in the audit, the total fee will be 609 USDT.

mydogmaxuk commented 3 years ago

Please include the import files - 609 USDT - Thank you

yuriy77k commented 3 years ago

the wallet address for ERC20 USDT is 0xb9662e592f2f0412be62f0833ca463a9b1aabebb

mydogmaxuk commented 3 years ago

Payment sent

yuriy77k commented 3 years ago

The audit will be completed in 4 days

mydogmaxuk commented 3 years ago

If complete in 24 hours I will pay an extra 600 USDT, 48 hours I will pay a further 400 USDT and 72 hours then 200 extra, Thanks.

yuriy77k commented 3 years ago

The audit will be complete as soon as possible

MrCrambo commented 3 years ago

Auditing time 1 day.

danbogd commented 3 years ago

Auditing time: 24 -48 hours.

yuriy77k commented 3 years ago

@MrCrambo assigned

yuriy77k commented 3 years ago

@danbogd Not assign

yuriy77k commented 3 years ago

@mydogmaxuk please, describe the logic of NFTLootBox smart contract. I see two functions:

  1. Function submitBet(uint256 lootboxID, uint256 seed, uint256 bets) where user will pay for selected lootboxID it's price multiply by bets amount. The 90% of paid tokens will be burned and 10% transfer to feeAddress. What means seed argument in this function? It's not using in the logic of the function.
  2. Function redeemBulk(address junkAsset, uint256 junkAmount, address nftAsset, uint256[] calldata id, uint256[] calldata nftAmount, uint256 bet, uint8 v, bytes32 r, bytes32 s) it transfer sets of NFTs and junkAmount of junkAsset to some user who submit the bet before. But the contract does not contain the logic of selection NFTs, asset, and amount - it passed as arguments of function with authAddress signature (centralize).
  3. There are some functions redeemERC1155, redeemERC1155Bulk, redeemERC20, redeemBulkERC20 which are commented. If you no need them. better to delete them in final version
Big-Sock commented 3 years ago

Users purchase lootboxes using their client seed. the contract doesn't use this seed, but the server (which reads the event emitted) does. Since generating random numbers is very difficult on-chain we calculate winnings serverside. once we've determined the user's winnings, they can redeem those winnings through the redeemBulk function. the other functions are leftovers and will soon be deleted.

mydogmaxuk commented 3 years ago

as above thanks

yuriy77k commented 3 years ago

@mydogmaxuk link to audit report: https://gist.github.com/yuriy77k/973fc9abef65eda61cb31105dbfde624

yuriy77k commented 3 years ago

NFTLootBox Security Audit Report

1. Summary

NFTLootBox.com smart contract security audit report performed by Callisto Security Audit Department

Logic description by developer:

Users purchase lootboxes using their client seed. the contract doesn't use this seed, but the server (which reads the event emitted) does. Since generating random numbers is very difficult on-chain we calculate winnings server-side. once we've determined the user's winnings, they can redeem those winnings through the redeemBulk function.

2. In scope

Commit hash 56ad73a604226e481e78b2dc634001721203ad66

NFTLootbox.sol lib/Context.sol lib/SafeMath.sol lib/Ownable.sol lib/IERC20.sol lib/IERC1155.sol lib/ReentrancyGuard.sol lib/Address.sol

3. Findings

In total, 3 issues were reported including:

No critical security issues were found.

3.1. Wrong import path

Severity: compiler error (note)

Description

The imported files located in the /lib/ folder, but in the code /lib/ is missing that cause compiler error.

Recommendation

Use import like following:

import "./lib/Context.sol";
import "./lib/SafeMath.sol";
import "./lib/Ownable.sol";
import "./lib/IERC20.sol";
import "./lib/IERC1155.sol";
import "./lib/ReentrancyGuard.sol";

3.2. Zero address checking

Severity: low

Description

There is no zero address checking in functions: setTransferAddress, setAuthAddress, updateLootbox

Recommendation

Add zero address checking:

require( _address != address(0) );

3.3. Gas wasting

Severity: note

Description

The modifier nonReentrant is overused that will cause unnecessary gas usage.

Recommendation

The modifier nonReentrant may be removed from functions setTransferAddress, setAuthAddress, updateLootbox where no risk of re-entrance.

4. Conclusion

The audited smart contract can be deployed. Only low severity issues were found during the audit.

5. Revealing audit reports

https://gist.github.com/MrCrambo/f11c21632ca2b02d92d2e8ede91b6d19

mydogmaxuk commented 3 years ago

As promised 600 USDT bonus for speedy work. Same address? Thanks

yuriy77k commented 3 years ago

@mydogmaxuk yes, address the same. Thanks.

mydogmaxuk commented 3 years ago

Sent

yuriy77k commented 3 years ago

Thank you