---
# System prepended metadata

title: '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.
