Code Review: ApeToken

[storming0x]

scope:

• ApeToken.sol
• TokenAccessControl.sol

[storming0x]

Small Comment:

I think the contract extension on brownie points to this version points to 4.0.0-rc which is ok as implemented https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1ee939e7c4b2c1fe84e6f5b6a7c1ce5e7d91667f/contracts/utils/cryptography/ECDSA.sol#L53

but i checked the latest master and OZ changed the expectations a bit about this library so if this is the deployed version I think usage is correct.

[storming0x]

Small comment:

In yearn vaults we add this check

https://github.com/yearn/yearn-vaults/blob/2a28331438063a48684db4143ad293821671cc67/contracts/Vault.vy#L756

it ensures owner cant be zero, which im not able to see that it is a problem as implemented since ECDSA also checks signer is never zero in this version, but like i said a small change on signer library can break this assumptions in version, so is good to make the check explicit in any case in the token code.

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/ApeToken.sol#L42

[storming0x]

Small Suggestion:

I did not see in 4.0.0-rc that the OZ contracts checks for transfers to address 0x0, is this something we want to require as check on both transfers to avoid holders accidentally burning shares?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/ApeToken.sol#L59

[storming0x]

Small gas optimization?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/TokenAccessControl.sol#L21

putting the "!whitelistDisabled" check short circuits the check faster if the flag is turn on at some point without reading the whitelist address storage

[storming0x]

Question:

Not an issue per se, guessing the requirement for whitelist is for freezing funds of specific accounts? usually these access controls are also paired with upgradability, since the requirement here is to just freeze funds but not able to recover them at any point in time, or am i misunderstanding the design?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/TokenAccessControl.sol#L21

[storming0x]

Small comment:

Here the modifier the normal flow for token transfers is that the contract is not paused right?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/TokenAccessControl.sol#L20

Maybe a bit nitpicky, but usually modifiers state what is the condition for the scope of the method to go forward, so here maybe isNotPaused or notPaused may be better to read, as that is the normal flow for token transfers.

[storming0x]

Mostly small, nitpicks. Most of the code in the scope LGTM!

[cesarsld]

Question:

Not an issue per se, guessing the requirement for whitelist is for freezing funds of specific accounts? usually these access controls are also paired with upgradability, since the requirement here is to just freeze funds but not able to recover them at any point in time, or am i misunderstanding the design?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/TokenAccessControl.sol#L21

Design is to enable a whitelisted member to transfer tokens while the transfer feature is paused

[cesarsld]

Small Suggestion:

I did not see in 4.0.0-rc that the OZ contracts checks for transfers to address 0x0, is this something we want to require as check on both transfers to avoid holders accidentally burning shares?

https://github.com/coordinape/coordinape-protocol/blob/df88ddd07d99429d10c7577da639ff38ff6a1d2e/contracts/ApeProtocol/token/ApeToken.sol#L59

Unless I looked in the wrong place, OZ does revert if recipient is 0x0 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1ee939e7c4b2c1fe84e6f5b6a7c1ce5e7d91667f/contracts/token/ERC20/ERC20.sol#L212

[cesarsld]

Thank you @storming0x I will implement changes on your feedback!

[parseb]

I think @cesarsld is on point. 🤔 Line 60: return ERC20.transfer(_to, _amount); explicit ERC20. transfer uses _transfer() function which validates input not being default 0x0 address.
_transfer(msgSender(), recipient, amount); Condition checked by ERC20 line 211 & 212: require(sender != address(0), "ERC20: transfer from the zero address"); require(recipient != address(0), "ERC20: transfer to the zero address");

[storming0x]

Im good to close issue based on comments and discussions @cesarsld

[spydermonkee]

Thanks a lot, @storming0x for your review. Nice to meet you through this avenue, BTW :)

I'm ready to close this issue and we're basically ready for deploying the token contracts. There are only 4 commits since your last comment and they seem innocuous enough to me: https://github.com/coordinape/coordinape-protocol/commits/main/contracts/ApeProtocol/token

Do you have any last concerns about any of these new commits before we deploy this? thanks!

[storming0x]

https://github.com/coordinape/coordinape-protocol/commits/main/contracts/ApeProtocol/token

i saw the latest commits, dont see any security issue at first glance in latest changes.

Im good to close the issue based on latest changes.

[spydermonkee]

Thanks a lot, @storming0x!