[Optional] Session 1 Audit Report Discussion

[Robert-H-Leonard]

Audit report for this discussion is here on the Canto protocol: https://code4rena.com/reports/2022-11-canto/

Please leave a comment with your analysis of an vulnerability and/or questions and thoughts your have.

[Robert-H-Leonard]

There is a low vulnerability [L-01] caused by the amount of control the owner address has on the contract. If a malicious party (either a hacker or the devs building the protocol) got control of the owner address they would have control of the protocols fee distribution. Based on the protocol (Canto) docs they assume the owner is an EOA.

The recommended fixes here are:

• Add a timelock to any owner so their access doesn't last forever
• Possibly build in a mechanism for a fallback. owner

This is a common thing to be thinking about when auditing. If there are roles that give addresses super admin privileges or if a contract is upgradable then protocols need to be super careful about how to build those mechanism. Also probably build circuit break failure paths if things do go wrong.

[aldrinmayen]

Low vulnerability [L-02] requires to have a safer way to transfer ownership of the contract by using a 2 step method.
the recommended fix is:

• to update to a different import link Ownable2step.sol
  this new function uses two functions transferOwnership() and acceptOwnership()

• please note that although this a recommended Ownable.sol file from OpenZeppeling; it seems there are other version out there that other projects are using.

[aldrinmayen]

Low vulnerability [L-03] "Owner can renounce ownership" this requires to disable some privileges from the owner of the contract; since the import code Ownable.sol link mentiones the function renounceOwnership function that could leave the contract without owner even if it is not by design'

• the recomend fix is to update the function distributionFees() to renounce ownership or eliminate that according to design.

however the recommendation is confusing since it is recommended to use Ownable2steps.sol in L-02 and if that is the case, there is no renounceOwnership function to call or perhaps, since Ownable2step is also Ownable, it could inherited renounceOwnership function? @Robert-H-Leonard @wzrdk3lly

[thor4]

[L-06] No Check if OnErc721Received is implemented
This finding highlights the fact that the Turnstile.register() function doesn't check whether the address receiving a newly minted NFT has the onERC721Received() function implemented. This is necessary so that the contract is able to take receipt of the NFT in a manner that will allow it to be transferred out at a later time. Currently, if an NFT is minted to a contract without onERC721Received() implemented, that contract will show up as the owner of the NFT. However, it will be stuck in there because it doesn't have the necessary code to implement proper handling of NFT contracts.
The recommended fix is to implement _safeMint instead of _mint because it checks to ensure the receiving contract has implemented onERC721Received() prior to carrying out the NFT mint.