QA Report

[code423n4]

Summary

Low Risk Issues

	Issue	Instances
1	tokenURI() shows invalid premium data	1
2	Final vault mintable locks asset forever	1
3	Cally does not follow ERC721 standard	1
4	Base64.encode() broken on L2 Arbitrum	1

Total: 4 instances over 4 issues

Non-critical Issues

	Issue	Instances
1	Not following best practice of using _safeMint()	1
2	Not following best practice of using safeTransferFrom() for NFTs	2
3	Sellers should be allowed to list at the reserve strike	1
4	constants should be defined rather than using magic numbers	14
5	Missing event for critical parameter change	1
6	Typos	3
7	File is missing NatSpec	2
8	NatSpec is incomplete	3
9	Event is missing indexed fields	1
10	Consider adding the ability to auto-exercise at expiration	1
11	Consider using a two-step ownership transfer model	1
12	Consider making vault creation pausable	1

Total: 31 instances over 12 issues

Low Risk Issues

1. tokenURI() shows invalid premium data

getPremium() takes in a vaultId not a premium index

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

464:             getPremium(vault.premiumIndex),

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L464

2. Final vault mintable locks asset forever

vaultId is a uint256 and type(uint256).max is odd and therefore a valid vaultId. optionIds are always one more than the vaultId, and for that case, the addition will cause an overflow. The overflow happens for both buying and withdrawing, so once added, assets are stuck forever in that specific vault

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

333          uint256 optionId = vaultId + 1;
334          _burn(optionId);
335:         _burn(vaultId);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L333-L335

3. Cally does not follow ERC721 standard

balanceOf() returns the wrong value for all users. It's not reasonable to disregard the standard to just save gas. The purpose of a standard is to ensure interoperability, and this current code breaks that. We file lots of medium-risk issues on various projects because Tether (USDT) does things in a non-standard way which leads to bugs in other people's code because they don't know about the idiosyncrasies. This project is introducing yet another potential attack vector.

There is 1 instance of this issue:

File: contracts/src/CallyNft.sol   #1

35       function balanceOf(address owner) public pure override returns (uint256) {
36           require(owner != address(0), "ZERO_ADDRESS");
37:          return type(uint256).max;

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/CallyNft.sol#L35-L37

4. Base64.encode() broken on L2 Arbitrum

See this issue filed against the source project. Feel free to downgrade to non-critical if Cally is only meant for Ethereum L1. Use OpenZeppelin's Base64.encode() instead

There is 1 instance of this issue:

File: contracts/lib/base64/base64.sol   #1

48                   // write 4 characters
49                   mstore8(resultPtr, mload(add(tablePtr, and(shr(18, input), 0x3F))))
50                   resultPtr := add(resultPtr, 1)
51                   mstore8(resultPtr, mload(add(tablePtr, and(shr(12, input), 0x3F))))
52                   resultPtr := add(resultPtr, 1)
53                   mstore8(resultPtr, mload(add(tablePtr, and(shr( 6, input), 0x3F))))
54                   resultPtr := add(resultPtr, 1)
55                   mstore8(resultPtr, mload(add(tablePtr, and(        input,  0x3F))))
56:                  resultPtr := add(resultPtr, 1)

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/lib/base64/base64.sol#L48-L56

Non-critical Issues

1. Not following best practice of using _safeMint()

The code should allow recipients to reject NFTs, by calling _safeMint(), which triggers transfer hooks in the recipient

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

193:         _mint(msg.sender, vaultId);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L193

2. Not following best practice of using safeTransferFrom() for NFTs

The code should allow recipients to reject NFTs, by calling safeTransferFrom(), which triggers transfer hooks in the recipient

There are 2 instances of this issue:

File: contracts/src/Cally.sol   #1

295:             ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L295

File: contracts/src/Cally.sol   #2

344:             ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344

3. Sellers should be allowed to list at the reserve strike

The condition below should use <= rather than <

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

169:         require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L169

4. constants should be defined rather than using magic numbers

There are 14 instances of this issue:

File: contracts/src/Cally.sol

/// @audit 1e18
284:              fee = (msg.value * feeRate) / 1e18;

/// @audit 1e18
418:          uint256 progress = (1e18 * delta) / AUCTION_DURATION;

/// @audit 1e18
419:          uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

/// @audit 1e18
419:          uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol

File: contracts/src/CallyNft.sol

/// @audit 4
245:              str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))];

/// @audit 3
246:              str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))];

/// @audit 0x0f
246:              str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))];

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/CallyNft.sol

File: contracts/lib/base64/base64.sol

/// @audit 4
22:           uint256 encodedLen = 4 * ((data.length + 2) / 3);

/// @audit 3
22:           uint256 encodedLen = 4 * ((data.length + 2) / 3);

/// @audit 32
25:           string memory result = new string(encodedLen + 32);

/// @audit 4
72:           require(data.length % 4 == 0, "invalid base64 decoder input");

/// @audit 4
78:           uint256 decodedLen = (data.length / 4) * 3;

/// @audit 3
78:           uint256 decodedLen = (data.length / 4) * 3;

/// @audit 32
81:           bytes memory result = new bytes(decodedLen + 32);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/lib/base64/base64.sol

5. Missing event for critical parameter change

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

119       function setFee(uint256 feeRate_) external onlyOwner {
120           feeRate = feeRate_;
121:      }

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119-L121

6. Typos

There are 3 instances of this issue:

File: contracts/src/Cally.sol   #1

/// @audit lifecycle
135:          standard lifecycle:

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L135

File: contracts/src/Cally.sol   #2

/// @audit lifecycle
145:          [*] can be called anytime in lifecycle

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L145

File: contracts/src/Cally.sol   #3

/// @audit OVVERIDES
426:          OVVERIDES FUNCTIONS

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L426

7. File is missing NatSpec

There are 2 instances of this issue:

File: contracts/src/CallyNft.sol (various lines)   #1

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/CallyNft.sol

File: contracts/lib/hot-chain-svg/contracts/SVG.sol (various lines)   #2

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/lib/hot-chain-svg/contracts/SVG.sol

8. NatSpec is incomplete

There are 3 instances of this issue:

File: contracts/src/Cally.sol   #1

/// @audit Missing: '@return'
156       /// @param dutchAuctionReserveStrike The reserve strike for each dutch auction
157       /// @param tokenType The type of the underlying asset (NFT or ERC20)
158       function createVault(
159           uint256 tokenIdOrAmount,
160           address token,
161           uint8 premiumIndex,
162           uint8 durationDays,
163           uint8 dutchAuctionStartingStrikeIndex,
164           uint256 dutchAuctionReserveStrike,
165           TokenType tokenType
166:      ) external returns (uint256 vaultId) {

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L156-L166

File: contracts/src/Cally.sol   #2

/// @audit Missing: '@return'
205       ///         vault beneficiary.
206       /// @param vaultId The tokenId of the vault to buy the option from
207:      function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L205-L207

File: contracts/src/Cally.sol   #3

/// @audit Missing: '@return'
385       /// @notice Get details for a vault
386       /// @param vaultId The tokenId of the vault to fetch the details for
387:      function vaults(uint256 vaultId) external view returns (Vault memory) {

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L385-L387

9. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

56:       event Harvested(address indexed from, uint256 amount);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L56

10. Consider adding the ability to auto-exercise at expiration

Most equity brokers automatically exercise options if they're in the money at expiration. It would be good to allow hooking up WETH and attempting to transfer and unwrap if still set at expiration

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

243          // force transfer the vault's associated option from old owner to new owner
244          // option id for a respective vault is always vaultId + 1
245          optionId = vaultId + 1;
246:         _forceTransfer(msg.sender, optionId);

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L243-L246

11. Consider using a two-step ownership transfer model

Having a two-step ownership transfer is safer because it ensures the new owner is actually valid

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

32:  contract Cally is CallyNft, ReentrancyGuard, Ownable {

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L32

12. Consider making vault creation pausable

This can help mitigate issues where a vulnerability is found after deployment

There is 1 instance of this issue:

File: contracts/src/Cally.sol   #1

158:     function createVault(

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L158

[outdoteth]

High quality report