# Security Audit of `Demotapers`


## Conclusion

This audit was made by [Web3Go](https://web3go.tech)
Auditor: Vladimir Smelov <[e-mail]>.
Date: 2022-10-14

In the final contract were **NOT FOUND**:
- Backdoors for investor funds withdrawal by anyone.
- Bugs allow stealing money from the contract.
- Any severe security problems.

There were **FOUND**:
- Places for beautification (typos, refactoring).
- Places for gas optimization.

The client was acknowledged about all notes below.
All required fixes were done.

**The code is ready for use.**

## Scope

#### Pre-audit

- DemotapersMarket.sol, md5 hash: 15087969b0afbb82ca857566c92b7dad
- DemotapersNFT.sol, md5 hash: 8e4f1ab4455ece7321412a6417cfc06e
- DemotapersNFTTest.sol, md5 hash: d7fe1a1b2387f27c515eb2137ff27ad4
- ERC165.sol, md5 hash: d336cb34366e7303e3f82b2beba86921

#### Final audit

TO BE DONE

## Methodology

1. Blind audit. Understand the structure of the code without reading any docs.
2. Ask questions to developers.
3. Draw the scheme of cross-contracts interactions.
4. Write user stories and usage cases.
5. Run static analyzers.
6. Find problems with:
- backdoors;
- bugs;
- math;
- potential leaking of funds;
- potential locking of the contract;
- validate arguments and events;
- others.

## Result

### Critical

Not found.

___

### Major

Not found.

___

### Warning

#### WARNING-1. Centralization power - The owner may change metadata.

- src/contracts/DemotapersNFT.sol:123

At any moment, the owner may turn all tokens into something completely unusable just by changing the baseURI value.

##### Recommendation

Use Gnosis Multisig. Consider the usage of individual tokenURI with immutable decentralized storage (like IPFS).

##### Status
ACKNOWLEDGED

___

### Comment.

#### COMMENT-1. Consider the usage of off-chain sales.

At:
- src/contracts/DemotapersNFT.sol
- src/contracts/DemotapersMarket.sol

To sell NFT to someone for a specific price, there should be

1. Mint NFT by owner
2. listItems by owner
3. buyItem by user

Every operation costs gas and takes time.

You can publish to your website (covered by the frontend) ECDSA signatures for not-minted-yet NFT with signing (tokenId, price, deadline).

Then if someone wants to get NFT, he calls:

function buyAndMint(tokenId, price, deadline, ownerSignature) external payable {
    _verifySignature(tokenId, price, deadline, ownerSignature);
    require(msg.value == price);
    require(block.timestamp < deadline);
    _mint(tokenId, msg.sender);
}

In that way, the owner pays no gas for minting and listing.

Similar mechanics are used in well-known OpenSea and Rarible.

##### Recommendation

Consider the usage of listing via off-chain signatures.

##### Status
ACKNOWLEDGED

The client wants to first mint and then sell NFTs.

---

#### COMMENT-2. Calculate signatures at compilation time rather than have them hard-coded.

At:
- src/contracts/ERC165.sol:9
- src/contracts/DemotapersNFT.sol:32-38

You have hard-coded constants like

_INTERFACE_ID_ERC165 = 0x01ffc9a7;

Any typo in any symbol will lead to an untrackable mistake, which will be impossible to find.

##### Recommendation

Consider rewriting to human-readable form:

_INTERFACE_ID_ERC165 = bytes4(keccak256('supportsInterface(bytes4)')); // 0x01ffc9a7 

##### Status
FIXED

The double check of "magic" hashes was added.

---

#### COMMENT-3. ERC165 implementation is gas-costly.

At:
- src/contracts/ERC165.sol:17

You read storage to check if the contract supports the interface or not.

function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
    return _supportedInterfaces[interfaceId];
}

It costs 2100 gas - https://github.com/wolflo/evm-opcodes/blob/main/gas.md#a6-sload

##### Recommendation

Consider rewriting to OpenZeppelin style with inheritance and OR operators.

##### Status
ACKNOWLEDGED

Not a big issue since, interface registry is done only once and view methods are not to be used intensevily.

---

#### COMMENT-4. Emit events on important state changes.

At:
- src/contracts/DemotapersNFT.sol:124
- src/contracts/DemotapersMarket.sol:155

Some important state variable is changed, but you cannot track them since you don't emit an event.

##### Recommendation

Emit event on essential state changes.

##### Status
ACKNOWLEDGED

These actions are done by the contract Owner, so no monitoring is needed.

---

#### COMMENT-5. Useless structure.

At:
- src/contracts/DemotapersMarket.sol:35

The structure has only one attribute, so it's useless.

##### Recommendation

Remove the structure.

##### Status
FIXED

---

#### COMMENT-6. Use `immutable` declarations.

At:
- src/contracts/DemotapersMarket.sol:39

Declare the variable as `immutable`; it saves gas.

##### Recommendation

Use `immutable` declarations.

##### Status
FIXED

---

#### COMMENT-7. Explicitly state the interface of the variable.

At:
- src/contracts/DemotapersMarket.sol:39

Declare the variable as `IERC721` for clarity.

##### Recommendation

Consider refactoring.

##### Status
FIXED

---

#### COMMENT-8. Use the `unchecked` declaration for index increment.

At:
- src/contracts/DemotapersMarket.sol:47
- src/contracts/DemotapersMarket.sol:66
- src/contracts/DemotapersMarket.sol:98
- src/contracts/DemotapersMarket.sol:110
- src/contracts/DemotapersNFT.sol:158

You have default solidity 0.8.x safeMath enabled for an increment:

for (uint i=0; i < tokenIds.length; i++) {
    ...
}

But you know for sure that `i` will never overflow.

##### Recommendation

Consider explicitly unchecking the increment to save gas:

for (uint i=0; i < tokenIds.length;) {
    unchecked{i++;}
    ...
}

##### Status
ACKNOWLEDGED

Skipped for the code clarity.

---

#### COMMENT-9. Avoid double SLOAD.

Whenever you call
- `cancelListing`
- `updateListing
- `buyItem`

You first read the storage in the modifier; then, you SLOAD the same storage slot in the function itself.

##### Recommendation

Consider the removal of modifiers and do all checks in one for-loop with one SLOAD to save gas.

##### Status
ACKNOWLEDGED

Skipped to avoid the logic duplication.

---

#### COMMENT-10. Prevent sending extensive ether.

At:
- src/contracts/DemotapersMarket.sol:139

It's better to require strictly.

if (msg.value != listedItem.price) {
    revert PriceNotMet(tokenId, listedItem.price);
}

To prevent user mistakes.

##### Recommendation

Consider using a strict check.

##### Status
FIXED

---

#### COMMENT-11. Never use `payable(...).transfer`.

At:
- src/contracts/DemotapersMarket.sol:154

You use the old-fashion style of sending the ethers.
See - https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

##### Recommendation

Use `payable{value: value}.call("");`

##### Status
FIXED

---

#### COMMENT-12. Add recoverERC20 and recoverERC721 to prevent user mistakes.

Users often make mistakes, occasionally directly sending wETH to the contract or sending NFT to the contract.

##### Recommendation

Consider adding
`recoverERC20`, `recoverERC721` to recover tokens sent to the contract by mistake.
##### Status
FIXED

---

#### COMMENT-13. Don't use `uint256(x) <= 0`

- src/contracts/DemotapersMarket.sol:59
- src/contracts/DemotapersMarket.sol:69
- src/contracts/DemotapersMarket.sol:91
- src/contracts/DemotapersMarket.sol:123

uint256 may never be under `0`; it's confusing to check `<= 0`.

##### Recommendation

Consider replacing with `== 0`.
##### Status
FIXED

---

#### COMMENT-14. Remove useless stack variables.

- src/contracts/DemotapersMarket.sol:94

You don't need a separate stack variable.

##### Recommendation

Consider rewriting:

if (!IERC721(nft).isApprovedForAll(msg.sender, address(this))) {

##### Status
FIXED

---

#### COMMENT-15. Import OpenZeppelin code.

- src/contracts/DemotapersNFT.sol:16-24

you can import
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/IERC721Metadata.sol
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721Receiver.sol

Also, you can inherit 80% of the code of DemotapersNFT from
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol

##### Recommendation

Consider importing OpenZeppelin code.

##### Status
ACKNOWLEDGED

---

#### COMMENT-16. Rename symbol "DMT".

- src/contracts/DemotapersNFT.sol:42

"DMT" is a well-known drug (type "DMT" in Google).

##### Recommendation

Consider renaming.

##### Status
FIXED

---

#### COMMENT-17. Unused `_marketAddress` variable.

- src/contracts/DemotapersNFT.sol

you never use `_marketAddress`, `setMarketAddress`, `getMarketAddress` for any business logic.

##### Recommendation

Consider removing.

##### Status
FIXED

---

#### COMMENT-18. Use `tokenId` rather than `tokenIndex`.

- src/contracts/DemotapersNFT.sol

You use variables named `tokenIndex`; however, the standard EIP-721 defines them as `tokenId`.

See - https://eips.ethereum.org/EIPS/eip-721

##### Recommendation

Consider renaming.

##### Status
FIXED

---

#### COMMENT-19. Fix typo in `tokenOowner`.

- src/contracts/DemotapersNFT.sol

##### Recommendation

Consider renaming it to `tokenOwner`.

##### Status
FIXED

---

#### COMMENT-20. Avoid calling `totalSupply` in a loop.

- src/contracts/DemotapersNFT.sol:159

`totalSupply()` is a costly operation.

##### Recommendation

Consider replacing with `first+i`

##### Status
FIXED

---

#### COMMENT-21. Remove the Context usage.

- src/contracts/DemotapersNFT.sol

You never switch the context, so you don't need to use `_msgSender()`

##### Recommendation

Consider replacing it with `msg.sender` to save gas.

##### Status
FIXED

---

#### COMMENT-22. Optimize `_approve`.

- src/contracts/DemotapersNFT.sol:399

You load `owner()`, but you already loaded it when you called the function.

##### Recommendation

Avoid double SLOAD.

##### Status
ACKNOWLEDGED

---

#### COMMENT-23. Move test and mock contracts to a separate folder.

- src/contracts/DemotapersNFTTest.sol

##### Recommendation

Consider moving this contract to a separate folder, "mocks" or "tests."

##### Status
FIXED