Cally contest - shung's results

Lines of code

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

Vulnerability details

Impact

This vulnerability allows the protocol owner to claim user deposited funds (vaults) at the cost of only premium, bypassing strike price. It requires that the option of the vault is buyable.

Proof of Concept

1. Alice creates vault (createVault()) for a BAYC NFT knowing that the protocol has no strike fees (feeRate == 0),
2. Protocol owner buys the option of the vault (buyOption()), with a strike price of 100 ETH, by paying the premium (say 0.1 ETH),
3. Protocol owner sets protocol fee rate to 100% (setFee(1e18)),
4. Protocol owner exercises the option (exercise()) by paying 100 ETH strike price.
5. Protocol owner can then claim back (withdrawProtocolFees()) the 100 ETH because 100% of the strike price is considered as protocol fee.

So in the end Alice loses her BAYC NFT and walks away with only 0.1 ETH, and the protocol owner walks away with a 100 ETH worth BAYC NFT by just paying 0.1 ETH.

Tools Used

Pen and paper.

Recommended Mitigation Steps

Simple Solution

Have a require statement in setFee() function that limits the fee rate to a reasonable amount (e.g. 10%).

function setFee(uint256 feeRate_) external onlyOwner {
    require(feeRate_ <= 1e17, "fee rate too high");
    feeRate = feeRate_;
}

Fully Trustless Solution

Although the simple solution presented above prevents the vulnerability, it still poses a centralization issue as the fees can be changed after a user creates a vault. I suggest snapshotting the fee rates such that when an option is exercised, its fee rate will be equal to the fee rate when the auction was last started for that vault.

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;

Gas Report

I have listed few optimizations for Cally.sol in the this report. The changes were tested by using the provided MockERC20.sol contract for the vaults. Compiler optimizer was enabled with 1000 runs. The table below shows the gas savings after applying all the suggested changes in this report.

createVault optimizations

1. It is more efficient to use storage pointer for vault instead of stashing it in memory. By using a storage pointer, there is no need to initialize struct properties to their default values.
2. At the end of the function, certain variables are accessed through the vault object. This wastes gas as these variables were already supplied as function arguments so they can be accessed directly. For example, instead of using vault.tokenType, you can directly use tokenType.
3. If the supplied argument dutchAuctionStartingStrikeIndex is not an index of strikeOptions, it will revert with an out of bounds error. Therefore there is no need to have an explicit require check for dutchAuctionStartingStrikeIndex < strikeOptions.length.

See below for an example of how I saved 1657 (1%) gas by applying these changes.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 872e4d7..65ae264 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -165,29 +165,23 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         TokenType tokenType
     ) external returns (uint256 vaultId) {
         require(premiumIndex < premiumOptions.length, "Invalid premium index");
-        require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
         require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
         require(durationDays > 0, "durationDays too small");
         require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");

-        Vault memory vault = Vault({
-            tokenIdOrAmount: tokenIdOrAmount,
-            token: token,
-            premiumIndex: premiumIndex,
-            durationDays: durationDays,
-            dutchAuctionStartingStrikeIndex: dutchAuctionStartingStrikeIndex,
-            currentExpiration: uint32(block.timestamp),
-            isExercised: false,
-            isWithdrawing: false,
-            tokenType: tokenType,
-            currentStrike: 0,
-            dutchAuctionReserveStrike: dutchAuctionReserveStrike
-        });
-
         // vault index should always be odd
         vaultIndex += 2;
         vaultId = vaultIndex;
-        _vaults[vaultId] = vault;
+        Vault storage vault = _vaults[vaultId];
+
+        vault.tokenIdOrAmount = tokenIdOrAmount;
+        vault.token = token;
+        vault.premiumIndex = premiumIndex;
+        vault.durationDays = durationDays;
+        vault.dutchAuctionStartingStrikeIndex = dutchAuctionStartingStrikeIndex;
+        vault.currentExpiration = uint32(block.timestamp);
+        vault.tokenType = tokenType;
+        vault.dutchAuctionReserveStrike = dutchAuctionReserveStrike;

         // give msg.sender vault token
         _mint(msg.sender, vaultId);
@@ -195,9 +189,9 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         emit NewVault(vaultId, msg.sender, token);

         // transfer the NFTs or ERC20s to the contract
-        vault.tokenType == TokenType.ERC721
-            ? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
-            : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
+        tokenType == TokenType.ERC721
+            ? ERC721(token).transferFrom(msg.sender, address(this), tokenIdOrAmount)
+            : ERC20(token).safeTransferFrom(msg.sender, address(this), tokenIdOrAmount);
     }

buyOption optimizations

1. It is more efficient to use storage pointer for vault instead of stashing it in memory. Stashing to memory requires accessing _vaults[vaultId] slot twice: once when reading, and once when writing.
2. Calling getPremium(vaultId) public function results in recalculation of _vaults[vaultId]. Since it is a small and trivial function, I suggest simply using premiumOptions[vault.premiumIndex].
3. vault.currentExpiration is assigned to auctionStartTimestamp, but later down the function vault.currentExpiration is used again as an argument of getDutchAuctionStrike. Simply reusing auctionStartTimestamp will save gas.

See below for an example of how I saved 3906 (3.2%) gas by applying these changes.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 65ae264..8115eba 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -199,7 +199,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
     ///         vault beneficiary.
     /// @param vaultId The tokenId of the vault to buy the option from
     function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {
-        Vault memory vault = _vaults[vaultId];
+        Vault storage vault = _vaults[vaultId];

         // vaultId should always be odd
         require(vaultId % 2 != 0, "Not vault type");
@@ -214,8 +214,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         require(vault.isWithdrawing == false, "Vault is being withdrawn");

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

         // check option associated with the vault has expired
         uint32 auctionStartTimestamp = vault.currentExpiration;
@@ -224,16 +223,13 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         // set new currentStrike
         vault.currentStrike = getDutchAuctionStrike(
             strikeOptions[vault.dutchAuctionStartingStrikeIndex],
-            vault.currentExpiration + AUCTION_DURATION,
+            auctionStartTimestamp + AUCTION_DURATION,
             vault.dutchAuctionReserveStrike
         );

         // set new expiration
         vault.currentExpiration = uint32(block.timestamp) + (vault.durationDays * 1 days);

-        // update the vault with the new option expiration and strike
-        _vaults[vaultId] = vault;
-
         // force transfer the vault's associated option from old owner to new owner
         // option id for a respective vault is always vaultId + 1
         optionId = vaultId + 1;

exercise optimizations

1. Once again, it is more efficient here to use storage pointer for vault instead of memory.

This simple change saves 3457 (~4.3%) gas.

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 8115eba..2cc19f9 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -253,7 +253,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         require(msg.sender == ownerOf(optionId), "You are not the owner");

         uint256 vaultId = optionId - 1;
-        Vault memory vault = _vaults[vaultId];
+        Vault storage vault = _vaults[vaultId];

         // check option hasn't expired
         require(block.timestamp < vault.currentExpiration, "Option has expired");
@@ -266,7 +266,6 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {

         // mark the vault as exercised
         vault.isExercised = true;
-        _vaults[vaultId] = vault;

         // collect protocol fee
         uint256 fee = 0;

Storage array optimizations

Storage arrays premiumOptions and strikeOptions are defined as dynamic arrays, but there are not admin functions that can change the array. Declaring them fixed length would be more efficient.

This optimization saves 4254 (2.7%) extra gas on createVault(), and 4316 (3.7%) extra gas on buyOption().

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 2cc19f9..49180a9 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -87,9 +87,9 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
     uint32 public constant AUCTION_DURATION = 24 hours;

     // prettier-ignore
-    uint256[] public premiumOptions = [0.01 ether, 0.025 ether, 0.05 ether, 0.075 ether, 0.1 ether, 0.25 ether, 0.5 ether, 0.75 ether, 1.0 ether, 2.5 ether, 5.0 ether, 7.5 ether, 10 ether, 25 ether, 50 ether, 75 ether, 100 ether];
+    uint256[17] public premiumOptions = [0.01 ether, 0.025 ether, 0.05 ether, 0.075 ether, 0.1 ether, 0.25 ether, 0.5 ether, 0.75 ether, 1.0 ether, 2.5 ether, 5.0 ether, 7.5 ether, 10 ether, 25 ether, 50 ether, 75 ether, 100 ether];
     // prettier-ignore
-    uint256[] public strikeOptions = [1 ether, 2 ether, 3 ether, 5 ether, 8 ether, 13 ether, 21 ether, 34 ether, 55 ether, 89 ether, 144 ether, 233 ether, 377 ether, 610 ether, 987 ether, 1597 ether, 2584 ether, 4181 ether, 6765 ether];
+    uint256[19] public strikeOptions = [1 ether, 2 ether, 3 ether, 5 ether, 8 ether, 13 ether, 21 ether, 34 ether, 55 ether, 89 ether, 144 ether, 233 ether, 377 ether, 610 ether, 987 ether, 1597 ether, 2584 ether, 4181 ether, 6765 ether];

     uint256 public feeRate = 0;
     uint256 public protocolUnclaimedFees = 0;

Struct packing

The vault struct can be further packed by making currentStrike and dutchAuctionReserveStrike both uint128. It is extremely unrealistic that ether supply will ever overflow 128 bits.

This change saves significant gas on buyOption (19014 gas) by making two storage writes into one. But it has negligible extra gas cost on createVault() (107 gas) and exercise() (18 gas).

diff --git a/contracts/Cally.sol b/contracts/Cally.sol
index 49180a9..03b4df3 100644
--- a/contracts/Cally.sol
+++ b/contracts/Cally.sol
@@ -80,8 +80,8 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         bool isExercised;
         bool isWithdrawing;
         TokenType tokenType;
-        uint256 currentStrike;
-        uint256 dutchAuctionReserveStrike;
+        uint128 currentStrike;
+        uint128 dutchAuctionReserveStrike;
     }

     uint32 public constant AUCTION_DURATION = 24 hours;
@@ -161,7 +161,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         uint8 premiumIndex,
         uint8 durationDays,
         uint8 dutchAuctionStartingStrikeIndex,
-        uint256 dutchAuctionReserveStrike,
+        uint128 dutchAuctionReserveStrike,
         TokenType tokenType
     ) external returns (uint256 vaultId) {
         require(premiumIndex < premiumOptions.length, "Invalid premium index");
@@ -396,7 +396,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         uint256 startingStrike,
         uint32 auctionEndTimestamp,
         uint256 reserveStrike
-    ) public view returns (uint256 strike) {
+    ) public view returns (uint128 strike) {
         /*
             delta = max(auctionEnd - currentTimestamp, 0)
             progress = delta / auctionDuration
@@ -408,7 +408,7 @@ contract Cally is CallyNft, ReentrancyGuard, Ownable {
         uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

         // max(auctionStrike, reserveStrike)
-        strike = auctionStrike > reserveStrike ? auctionStrike : reserveStrike;
+        strike = auctionStrike > reserveStrike ? uint128(auctionStrike) : uint128(reserveStrike);
     }

     /*************************