QA Report

[code423n4]

QA Report

vaultIndex inexplicably starts from 3

It does not make sense that the vault index starts from 3. If it was intended that the indexing starts from 1, you can simply move the incrementation one line below.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 872e4d7..aa78c09 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -185,8 +185,8 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         });

         // vault index should always be odd
-        vaultIndex += 2;
         vaultId = vaultIndex;
+        vaultIndex += 2;
         _vaults[vaultId] = vault;

         // give msg.sender vault token

NewVault event does not mention token type

Token type might be something useful for off-chain indexers and the user frontend.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index aa78c09..aac7cc1 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -37,7 +37,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
     /// @param vaultId The newly minted vault NFT
     /// @param from The account that created the vault
     /// @param token The token address of the underlying asset
-    event NewVault(uint256 indexed vaultId, address indexed from, address indexed token);
+    event NewVault(uint256 indexed vaultId, address indexed from, address indexed token, TokenType tokenType);

     /// @notice Fires when an option has been bought from a vault
     /// @param optionId The newly minted option NFT
@@ -192,7 +192,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         // give msg.sender vault token
         _mint(msg.sender, vaultId);

-        emit NewVault(vaultId, msg.sender, token);
+        emit NewVault(vaultId, msg.sender, token, tokenType);

         // transfer the NFTs or ERC20s to the contract
         vault.tokenType == TokenType.ERC721

Important state change does not emit event

Changing the protocol fee should emit an event because that is an important state variable.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index aac7cc1..2520840 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -65,6 +65,8 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
     /// @param from The account that is withdrawing
     event Withdrawal(uint256 indexed vaultId, address indexed from);

+    event ChangedFee(uint256 newFee);
+
     enum TokenType {
         ERC721,
         ERC20
@@ -118,6 +120,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
     /// @param feeRate_ The new fee rate: fee = 1% = (1 / 100) * 1e18
     function setFee(uint256 feeRate_) external onlyOwner {
         feeRate = feeRate_;
+        emit ChangedFee(feeRate_);
     }

     /// @notice Withdraws the protocol fees and sends to current owner

Too few elements in premiumOptions and strikeOptions

The range in premiumOptions and strikeOptions can be limiting. For example 0.01 ETH can be considered too high for a premium if ETH price increases by a lot. Also, there may be other EVM chains where the predefined options do not make sense for their prices.

I suggested in my gas report to make these arrays fixed length. If you decide to go with that approach, I suggeset increasing the element count by hardcoding into the contract to offer a higher range and option of prices.

If you do not plan to make the arrays fixed length, then the protocol owners can benefit from the ability to add more options by push functions as demonstrated below.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 2520840..7c21528 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -130,6 +130,14 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         payable(msg.sender).safeTransferETH(amount);
     }

+    function addPremiumOption(uint256 newPremiumOption) external onlyOwner {
+        premiumOptions.push(newPremiumOption);
+    }
+
+    function addStrikeOption(uint256 newStrikeOption) external onlyOwner {
+        strikeOptions.push(newStrikeOption);
+    }
+
     /**************************
         MAIN LOGIC FUNCTIONS
     ***************************/

Unbounded payment is prone to user errors

buyOption() allows unbounded payments. Allowing unbounded payment can result in user mistakes. It makes more sense here to require equality to not allow such mistakes.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 7c21528..0009573 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -232,7 +232,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {

         // check enough eth was sent to cover premium
         uint256 premium = getPremium(vaultId);
-        require(msg.value >= premium, "Incorrect ETH amount sent");
+        require(msg.value == premium, "Incorrect ETH amount sent");

         // check option associated with the vault has expired
         uint32 auctionStartTimestamp = vault.currentExpiration;

[outdoteth]

this can be bumped to medium severity:

Unbounded payment is prone to user errors: https://github.com/code-423n4/2022-05-cally-findings/issues/84

[outdoteth]

high quality report

[HardlyDifficult]

Per the C4 guidance "part of auditing is demonstrating proper theory of how an issue could be exploited" and that does not seem to be explored here as it was in the primary report.