Canto Identity Subprotocols contest - atharvasama's results

Low and Non-critical issues

[01] Solmate’s SafeTransferLib doesn’t check whether the ERC20 contract exists

Solmate’s SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn’t exist (yet).

This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

canto-namespace-protocol/src/Namespace.sol:

114:         SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, fusingCosts);

canto-namespace-protocol/src/Tray.sol:

6: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

157:             SafeTransferLib.safeTransferFrom(note, msg.sender, revenueAddress, _amount * trayPrice);

[02] Two steps verification before transferring ownership

Solmate's Auth.sol transfers ownership in a single step using the transferOwnership() function. Transferring of ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

The contracts using Auth.sol include

canto-namespace-protocol/src/Namespace.sol

canto-namespace-protocol/src/Tray.sol

[03] Typos

canto-pfp-protocol/src/ProfilePicture.sol

61:             // Register CSR on Canto mainnnet

canto-namespace-protocol/src/Namespace.sol

82:             // Register CSR on Canto mainnnet

canto-namespace-protocol/src/Tray.sol

95:     /// @param _revenueAddress Adress to send the revenue to

111:             // Register CSR on Canto mainnnet

247:         uint256 charRandValue = Utils.iteratePRNG(_seed); // Iterate PRNG to not have any biasedness / correlation between random numbers

canto-namespace-protocol/src/Utils.sol

7: /// @notice Utiltities for the on-chain SVG generation of the text data and pseudo randomness

[04] Use a more recent version of Solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Use a more recent solidity versions for the following contracts

canto-bio-protocol/src/Bio.sol

canto-namespace-protocol/src/Namespace.sol

canto-namespace-protocol/src/Tray.sol

canto-namespace-protocol/src/Utils.sol

canto-pfp-protocol/src/ProfilePicture.sol

[05] Avoid using tx.origin

tx.origin is a global variable in Solidity that returns the address of the account that sent the transaction.

Using the variable could make a contract vulnerable if an authorized account calls a malicious contract. You can impersonate a user using a third party contract.

canto-bio-protocol/src/Bio.sol

36:             turnstile.register(tx.origin);

canto-namespace-protocol/src/Namespace.sol:

84:             turnstile.register(tx.origin);

canto-namespace-protocol/src/Tray.sol:

113:             turnstile.register(tx.origin);

canto-pfp-protocol/src/ProfilePicture.sol

63:             turnstile.register(tx.origin);

[06] A single point of failure

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.

Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

canto-namespace-protocol/src/Namespace.sol

196:     function changeNoteAddress(address _newNoteAddress) external onlyOwner {

204:     function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {

canto-namespace-protocol/src/Tray.sol

210:     function changeNoteAddress(address _newNoteAddress) external onlyOwner {

218:     function changeRevenueAddress(address _newRevenueAddress) external onlyOwner {

225:     function endPrelaunchPhase() external onlyOwner {

Gas Optimizations list

Gas Optimizations

[G-01] CAN DECLARE VARIABLE OUTSIDE LOOP TO SAVE GAS

Declaring the stack variable outside the loop will save gas that would otherwise be spent on declaring the variable over and over again.

canto-bio-protocol/src/Bio.sol

57:             bytes1 character = bioTextBytes[i];

61:                 bytes1 nextCharacter;

canto-namespace-protocol/src/Namespace.sol

123:             bool isLastTrayEntry = true;
124:             uint256 trayID = _characterList[i].trayID;
125:             uint8 tileOffset = _characterList[i].tileOffset;

134:             uint8 characterModifier = tileData.characterModifier;

144:             bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);

146:             uint256 numBytesChar = charAsBytes.length;

canto-namespace-protocol/src/Tray.sol

160:             TileData[TILES_PER_TRAY] memory trayTiledata;

canto-namespace-protocol/src/Utils.sol

148:                 uint256 characterIndex = (_characterModifier % ZALGO_NUM_ABOVE) * 2;

157:                 uint256 characterIndex = (_characterModifier % ZALGO_NUM_OVER) * 2;

166:                 uint256 characterIndex = (_characterModifier % ZALGO_NUM_BELOW) * 2;

[G-02] AVOID CONTRACT EXISTENCE CHECKS BY USING LOW LEVEL CALLS

Before Solidity 0.8.10 the compiler inserted extra code to check for contract existence for external function calls EXTCODESIZE (100 gas). In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence.

canto-namespace-protocol/src/Namespace.sol

156:                 address trayOwner = tray.ownerOf(trayID);

[G-03] PUBLIC FUNCTIONS NOT CALLED BY THE CONTRACT SHOULD BE DECLARED EXTERNAL INSTEAD

Contracts are allowed to override their parents’ functions and change the visibility from public to external and can save gas by doing so.

canto-bio-protocol/src/Bio.sol

43:     function tokenURI(uint256 _id) public view override returns (string memory) {

canto-namespace-protocol/src/Namespace.sol

90:     function tokenURI(uint256 _id) public view override returns (string memory) {

canto-namespace-protocol/src/Tray.sol

119:     function tokenURI(uint256 _id) public view override returns (string memory) {

[G-04] OPTIMIZE NAMES TO SAVE GAS

Function hashes that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted. Prioritize the most called functions and sort and rename them according to the function hashes/method IDs. For a better understanding please refer to this link

A lower method ID may be given to the most frequently used functions. This is a useful tool that can be used for the same, if required.

[G-05] INTERNAL FUNCTIONS ONLY CALLED ONCE CAN BE INLINED TO SAVE GAS

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

canto-namespace-protocol/src/Tray.sol

231:     function _beforeTokenTransfers(
232:         address, /* from*/
233:         address to,
234:         uint256 startTokenId,
235:         uint256 /* quantity*/
236:     ) internal view override {

[G-06] BEFORE SOME FUNCTIONS, WE SHOULD CHECK SOME VARIABLES FOR POSSIBLE GAS SAVE

Before transfer, we should check for amount being 0 so the function doesnt run when its not gonna do anything.

canto-namespace-protocol/src/Tray.sol

150:     function buy(uint256 _amount) external {

[G-07] USAGE OF UINT/INT SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed.

canto-namespace-protocol/src/Namespace.sol

34:         uint8 tileOffset;

36:         uint8 skinToneModifier;

125:             uint8 tileOffset = _characterList[i].tileOffset;

134:             uint8 characterModifier = tileData.characterModifier;

canto-namespace-protocol/src/Tray.sol

59:         uint8 fontClass;

61:         uint16 characterIndex;

63:         uint8 characterModifier;

canto-namespace-protocol/src/Utils.sol

131:             uint8 asciiStartingIndex = 97; // Starting index for (lowercase) characters

138:             uint8 asciiStartingIndex = 97;

272:         uint8 lastByteVal = uint8(_startingSequence[3]);
273:         uint8 lastByteSum = lastByteVal + _characterIndex;

[G-08] USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value