search

CallistoSecurity / Smart-contract-auditing

This is a working repo of @EthereumCommonwealth audits. We performed more than 400 security audits since 2018. Not even a single contract was hacked after our auditors approved the code. Accepting audit requests here.
https://audits.callisto.network/
2 stars 2 forks source link

Live4well trade and transfer contract #15

Closed zerongt9 closed 1 year ago

zerongt9 commented 1 year ago

Audit request

Smart Contract for MarketPlace to support ERC20/ERC721 Payment and ERC721/ERC1155 <-> ERC20 trading

Source code

https://github.com/technine-IT/live4well-transfer-and-trade-smartcontractfor-audit

Payment plan

... Write [x] at the checkbox of the payment plan that suits your needs ...

Disclosure policy

... Do you want us to publish the report as it is or to notify you 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)

zero.ng@technine.io

Platform

... In which network will your contract be deployed? (EOS/TRX/ETC/ETH/CLO/UBQ/something else ) ...

yuriy77k commented 1 year ago

@zerongt9 The audit fee is 2400 USDT. You may send USDT (ERC20 or BEP20) to 0x6317c6944bd1cD3932d062cce39d7Fd602119529 (valid for Ethereum and Binance Smart Chain)

The estimated auditing time - is 7 days after payment.

zerongt9 commented 1 year ago

https://etherscan.io/tx/0xa6fb1e40f3063d3ef3347d6caa1c606ebef979fda242883d2b69e78c625fc627

yuriy77k commented 1 year ago

https://etherscan.io/tx/0xa6fb1e40f3063d3ef3347d6caa1c606ebef979fda242883d2b69e78c625fc627

Audit started

yuriy77k commented 1 year ago

Live4well Transfer and Trade Security Audit Report

1. Summary

Live4well Transfer and Trade smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit: 8d1da158a15099cb81e99dcaa5a554a0ae3bae25

3. Findings

In total, 1 issue were reported, including:

In total, 4 notes were reported, including:

3.1. TradingERC20 contract accepts only NFTs that implemented the royaltyInfo() function

Severity: medium

Description

If the NFT contract (ERC721 or ERC1155) does not implement the royaltyInfo() function, the calling of this function will revert the transaction.

Code snippet

Recommendation

if you want to accept any ERC721 and ERC1155, use following code:

        address royaltyRecipient;
        uint256 royaltyPrice;
        try nftContract.royaltyInfo(_tokenId, _erc20Amount) returns (address _royaltyRecipient, uint256 _royaltyPrice) {
            royaltyRecipient = _royaltyRecipient;
            royaltyPrice = _royaltyPrice;
        } catch (bytes memory) {}

3.2. Owner privileges

Severity: owner privileges

Description

  1. The owner of TransferContract can transfer tokens, which are approved by this contract, to any address or burn it.
  2. The owner of the TradingERC20 contract can grant TRADER_ROLE to any address. The address with TRADER_ROLE can transfer any approved tokens to any address (using fake NFT or ERC20 contracts).

Recommendation

Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract. Users should trust to owner.

3.3. Multiple minor observations

Severity: minor observation

Description

  1. The TransferContract and TradingERC20 contracts import @openzeppelin/contracts/access/Ownable.sol (so during compilation the latest version of Ownable.sol is used). But in the latest version of Ownable.sol the constructor requires argument address initialOwner. Therefore TransferContract and TradingERC20 contracts can't be compiled.

Code snippet

Recommendation

Specify the version of Openzeppelin contracts like: import @openzeppelin/contracts@4.9.0/ ... to avoid issues when a new version has incompatible functions.

  1. The event TradingRecord is declared but never used. Recommend to emit this event in the functions tradeERC721() and tradeERC1155().

Code snippet

  1. Checking allowance, balances, and approvals of transferred tokens is not necessary, because these checks are performed in the token contract during the transfer call.

Also, ERC721 can be approved in two ways: using functions approve or setApprovalForAll, but the function tradeERC721 checks only getApproved and omit isApprovedForAll.

Code snippet

Recommendation

Remove unnecessary checks to reduce transaction costs and improve compatibility.

3.4. Follow good coding practice

Severity: minor observation

Description

  1. Unlocked Pragma.

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.8.4, ensures that contracts do not accidentally get deployed using a compiler version with unfixed bugs.

  1. Missing docstrings.

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

  1. Missing test suite.

The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure that the contract functions and behaves as expected.

  1. Functions not used internally could be marked as external.

It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract.

4. Security practices

5. Conclusion

The audited smart contract must not be deployed. Reported issues must be fixed before the usage of this contract.

The audited contracts are centralized and users should pay attention to the owner's priviliges.

It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.

6. Revealing audit reports

zerongt9 commented 1 year ago

@yuriy77k

we have submitted the fix https://github.com/technine-IT/live4well-transfer-and-trade-smartcontractfor-audit

please help to arrange to re-audit

yuriy77k commented 1 year ago

Live4well Transfer and Trade Security v2 Audit Report

1. Summary

Live4well Transfer and Trade smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit: 5bcd3b748860d4085556511487350f155661e04c

3. Findings

In total, 0 issues were reported, including:

In total, 4 notes were reported, including:

3.1. Owner privileges

Severity: owner privileges

Description

  1. The owner of TransferContract can transfer tokens, which are approved by this contract, to any address or burn it.
  2. The owner of the TradingERC20 contract can grant TRADER_ROLE to any address. The address with TRADER_ROLE can transfer any approved tokens to any address (using fake NFT or ERC20 contracts).

Recommendation

Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract. Users should trust to owner.

3.2. Unnecessary checks

Severity: minor observation

Description

Checks of allowance, balances, and approvals of transferred tokens is not necessary, because these checks are performed in the token contract during the transfer call.

Code snippet

Recommendation

Remove unnecessary checks to reduce transaction costs.

3.3. Follow good coding practice

Severity: minor observation

Description

  1. Unlocked Pragma.

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.8.4, ensures that contracts do not accidentally get deployed using a compiler version with unfixed bugs.

  1. Missing docstrings.

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

  1. Missing test suite.

The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure that the contract functions and behaves as expected.

  1. Functions not used internally could be marked as external.

It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract.

4. Security practices

5. Conclusion

The audited smart contract can be deployed. No security issues were found during the audit.

The audited contracts are centralized and users should pay attention to the owner's privileges.

It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.

6. Previous audit reports