Jake's Audit Notes: GMO Adam NFT

# Jake's Audit Notes: GMO Adam NFT Client: GMO Adam Repo: https://github.com/ADAM-byGMO/contracts Commit: `9a1777596639fc07e9de349ee478fa224e1a09c9` ## Whitepaper & specification about the protocol No whitepaper on https://adam.jp/ or Jira. Background on the project at https://www.gmo.jp/en/news/article/811/ "GMO Internet Group (https://www.gmo.jp/en/ ) will realize an authentic, safe, and secure digital content payment and distribution, and support the online content distribution revolution utilizing a non-fungible token (NFT) by working on the development of Adam byGMO (https://adam.jp ), a new NFT marketplace for sellers and buyers to be launched in August 2021." Project is an NFT marketplace. ## Review of the protocol/implementation ### Adam721.sol [1] **Incorrect access control implementation** [`burn()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721.sol#L29) has no access modifier to control who may call the function. The check for the callers role is completed in a `require()` statement, but this is not best practice. Same applies in [`safeMint()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721.sol#L46), [`setBaseURI()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721.sol#L51), and [`setTokenURI()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721.sol#L56). Recommendations: [a] Use an access modifier to control who may call this function and limit `require()` checks for argument checking/external call validation. [2] **Misleading event emission on burn** [`burn()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721.sol#L29) emits an event `ChangeTokenURI()` once the token has been burnt. This is the same event that gets emitted on changes of the Token URI. Recommendations: [a] Consider emitting an event specific to burning operations. ### Adam721Factory.sol [1] **Incorrect access control implementation** [`createToken()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L22) has no access modifier to control who may call the function. The check for the callers role is completed in a `require()` statement, but this is not best practice. Same applies in [`grantRoleTokenBatch()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L32), [`revokeRoleTokenBatch()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L47), and [`setBaseURITokenBatch()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L62). Recommendations: [a] Use an access modifier to control who may call these functions and limit `require()` checks for argument checking/external call validation. [2] **Batch grant/revoke functions missing input validation** [`grantRoleTokenBatch()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L32) and [`revokeRoleTokenBatch()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Adam721Factory.sol#L47) have `grantRole()` and `revokeRole()` respectively. No checks are performed to validate if the roles being added are present or can be removed. Recommendations: [a] Confirm the roles granted and revoked to accounts are valid roles. ### AdamCustodianAdam721.sol [1] **Gas inefficient functions** [`callAdam721Burn()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianAdam721.sol#L25) and [`callAdam721SafeMintOrTransferFrom()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianAdam721.sol#L32) have `public` views. Recommendations: [a] Change function to views to be`external` for lower gas usage. [2] **Incorrect access control implementation** [`callAdam721Burn()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianAdam721.sol#L25) has no access modifier to control who may call the function. The check for the callers role is completed in a `require()` statement, but this is not best practice. Same applies in [`callAdam721SafeMintOrTransferFrom()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianAdam721.sol#L32). Recommendations: [a] Use an access modifier to control who may call these functions and limit `require()` checks for argument checking/external call validation. ### AdamCustodianERC721.sol [1] **Gas inefficient function** [`callERC721SafeTransferFrom()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianERC721.sol#L23) has a `public` view. Recommendations: [a] Change the function view to be external for lower gas usage. [2] **Incorrect access control implementation** [`callERC721SafeTransferFrom()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamCustodianERC721.sol#L23) has no access modifier to control who may call the function. The check for the callers role is completed in a `require()` statement, but this is not best practice. [a] Use an access modifier to control who may call these functions and limit require() checks for argument checking/external call validation. ### AdamEthSettlement.sol [1] **Unclear `settle()` function** [`settle()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamEthSettlement.sol#L27) is not well documented so it is hard to determine the intended implementation. It's not clear if `_sellerPrice` is actually the seller price as it's an argument passed into the function and does not have an external call to validate the price. Recommendations: [a] Document the intended implementation and usage. [2] **Access modifier inconsistency** The rest of the project uses `AccessControl` from OpenZeppelin to manage access control to external functions. `Ownable` has been used in this file which is an inconsistency. Moreover, the owner is not set in the constructor so the `onlyOwner` address is not defined. Affects [`withdrawERC20()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamEthSettlement.sol#L66), [`changeOperator()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamEthSettlement.sol#L73), and [`changeAdamWallet()`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/AdamEthSettlement.sol#L79). Recommendations: [a] Move to using `AccessControl` for function access. [b] Consider using a multi-sig wallet for protected functions instead of making admin changes using a single EOA account in the contracts. ### Migrations.sol [1] Unset Pragama version [`pragma`](https://github.com/ADAM-byGMO/contracts/blob/main/contracts/Migrations.sol#L2) compiler version is not set. This may lead to an unintended version used during deployment, with unexpected behaviour of the desired vs actual implementation. Recommendations: [a] Set to `pragma solidity 0.8.4;` as is consistent with the rest of the project. ## General Comments - Lack of doctrings on functions describing intented behaviour. Instructions in `README.md` are useful but should be docstrings instead. This helps to keep documentation consistent in the event of change to the code during development, or upgrades via proxy patterns later. - The documentation states there's a single EOA address for `DEFAULT_ADMIN_ROLE`. This should be a multi-sig account with multiple key holders.