Cally contest - joestakey's results

Lines of code

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

Vulnerability details

NO TIMELOCK ON SETFEE()

There is no timelock on setFee(). This is the fee that is applied in exercise(), and determines how much the Vault Creator is actually credited upon a call option being exercised. But:

• there is no maximum value limiting what fee can be set to
• there is no timelock, ie feeRate is set to its new value when setFee() is created.

Users will be incited to create vaults if the fee is low (feeRate is initially 0). A malicious owner could effectively wait for vaults being created and call options to be bought, then set a very high fee, which would result in any exercised call option sending all the strike ETH to the Cally owner.

Impact

Medium

Proof Of Concept

Let's take a similar example as in the Readme file:

• Alice creates a vault with 0.1 ETH premium, 30 day duration on her BAYC, max strike of 500 ETH
• Dutch auction starts at a strike of 500 ETH and decreases to 0 ETH over a 24 hour auction period
• Bob buys the call for 0.1 ETH, 30 day duration at a strike of 164.3 ETH after T amount of time has passed

The malicious owner now calls setFee(1e18), setting feeRate as 100%.

• Bob exercises his option after 23 days

The BAYC is sent to Bob. But because of the new fee, and of this part of the code, the strike amount is collected as protocol fees, and Alice's ETH balance does not get increased. Alice has effectively lost her BAYC.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Two things can be done:

• set a maximum possible fee rate in setFee()
• add a timelock in setFee(), ensuring all active options are not affected by the change in feeRate.

Lines of code

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

Vulnerability details

ERC721 TRANSFERFROM() MIGHT LEAD TO USER LOSING FUNDS

In exercise() and withdraw(), the ERC721 transferFrom() method is used to transfer the premium asset from the contract to the caller. If the caller is a contract that has not implemented the onERC721Received method properly, the NFT could be locked.

Impact

Medium

Proof Of Concept

Instances include:

line 258 and line 318

Tools Used

Manual Analysis

Recommended Mitigation Steps

use OZ's safeTransferFrom() instead of TransferFrom(). This way, if someone uses a contract to buy the call option and that contract has not implemented the ERC721 Receiving method properly, the execute() function will revert.

QA Report

Table of Contents

• summary
• Events emitted early
• Event should be emitted in setters
• Function missing comments
• Related data should be grouped in struct
• nonReentrant modifier unused
• High feeRate can break core protocol function
• Unchecked inputs

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the lack of checks in setters.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol: 195 emit NewVault(vaultId, msg.sender, token);//emitted before tokens transferred to contract.
Cally.sol: 291 emit ExercisedOption(optionId, msg.sender);//emitted before tokens transferred to user.
Cally.sol: 337 emit Withdrawal(vaultId, msg.sender);//emitted before harvest() call and tokens transferred to user.
Cally.sol: 365 emit Harvested(msg.sender, amount);//emitted before ETH transferred to user.

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:119: setFee(uint256 feeRate_)
Cally.sol:351: setVaultBeneficiary(uint256 vaultId, address beneficiary)

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:455: function tokenURI(uint256 tokenId)

CallyNft.sol

CallyNft.sol:51: function renderJSON()
CallySvg.sol:136: function renderSvg()
CallyNft.sol:236: function addressToString()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:103: mapping(uint256 => Vault) private _vaults;
Cally.sol:108: mapping(uint256 => address) private _vaultBeneficiaries;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, for the Cally.sol mappings, the mitigation could be:

struct VaultInformation {
  Vault _vault;
  address beneficiary;
}

And it would be used as a state variable:

mapping(uint256 =>  VaultInformation) private vaultInformation;

nonReentrant modifier unused

PROBLEM

Some external functions calling the ERC20 methods safeTransfer or safeTransferFrom do not have the nonReentrant modifier and are hence unprotected to reentrancy (besides the gas limit on the methods). No funds are directly at loss but it is best practice to avoid reentrancy altogether.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:158: function createVault()
Cally.sol:258: function exercise()
Cally.sol:368: function harvest()

TOOLS USED

Manual Analysis

MITIGATION

Use the nonReentrant modifier on these functions.

High feeRate can break core protocol function

PROBLEM

There is no maximum input value on setFee() in Cally.sol. But if the owner sets it to a uint greater than 1e18, the users will not be able to call exercice() as the function will revert, breaking the protocol's functionality.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:284: fee = (msg.value * feeRate) / 1e18;

If feeRate is set so that ethBalance[getVaultBeneficiary(vaultId)] + msg.value < fee, and the following statement will revert

Cally.sol:289: ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

TOOLS USED

Manual Analysis

MITIGATION

Add a check in setFee to ensure the new fee rate is less than a maximum maxFeeRate. Its value depends on different factors, but considering it determines how much ETH a vault creator is receiving from a strike, it should be reasonably low (ie less than 0.5 * 1e18)

Unchecked inputs

PROBLEM

Setters should check the input value - ie make revert if it is the zero address. Here, if the vault beneficiary is set as the zero address, all the strike ETH associated with the vault will be locked.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:351: setVaultBeneficiary()

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check

Gas Report

Table of Contents

• Check non zero values can avoid external call
• Comparisons with zero for unsigned integers
• Comparison operators
• Custom errors
• Default value initialization
• Prefix increments
• unchecked arithmetic
• unnecessary computation

Check non zero values can avoid external call

IMPACT

when calling ERC20 transfer functions, it is good to check the amount is non-zero, to avoid an unnecessary function call

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:158: function createVault(
        uint256 tokenIdOrAmount) //if it is an ERC20 token, it does not make sense to create a vault for 0 tokens.
Cally.sol:200 : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
Cally.sol:296 : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

TOOLS USED

Manual Analysis

MITIGATION

check that tokenIdOrAmount != 0 before these calls. For createVault() add it only in the case of an ERC20 (some ERC721 may have a token with an id of 0.)

Comparisons with zero for unsigned integers

IMPACT

>0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:170

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with !0

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:224: require(msg.value >= premium, "Incorrect ETH amount sent");
Cally.sol:228: require(block.timestamp >= auctionStartTimestamp, "Auction not started");

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-block.timestamp >= auctionStartTimestamp
+block.timestamp > auctionStartTimestamp - 1

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-block.timestamp >= auctionStartTimestamp
+block.timestamp > auctionStartTimestamp

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:167
Cally.sol:168
Cally.sol:169
Cally.sol:170
Cally.sol:171
Cally.sol:211
Cally.sol:214
Cally.sol:217
Cally.sol:220
Cally.sol:224
Cally.sol:228
Cally.sol:260
Cally.sol:263
Cally.sol:269
Cally.sol:272
Cally.sol:304
Cally.sol:307
Cally.sol:320
Cally.sol:323
Cally.sol:328
Cally.sol:329
Cally.sol:330
Cally.sol:353
Cally.sol:354
Cally.sol:436
Cally.sol:437
Cally.sol:438
Cally.sol:456

CallyNFT.sol

CallyNFT.sol:15
CallyNFT.sol:16
CallyNFT.sol:36
CallyNFT.sol:42

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in Cally.sol:

Replace

require(_ownerOf[tokenId] != address(0), "URI query for NOT_MINTED token");

with

if (_ownerOf[tokenId] == address(0)) {
		revert IsNotMinted(tokenId);
}

and define the custom error in the contract

error IsNotMinted(uint256 _tokenId_);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:94: uint256 public feeRate = 0;
Cally.sol:95: uint256 public protocolUnclaimedFees = 0;
Cally.sol:282: uint256 fee = 0;

CallyNft.sol

CallyNft.sol:244: uint256 i = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

CallyNft.sol

CallyNft.sol:244: i++

TOOLS USED

Manual Analysis

MITIGATION

change i++ to ++i.

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

CallyNft.sol

CallyNft.sol:244: i bounded by data.length, which is bounded as data.length = 20, overflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

If a value is known, it is more efficient to hardcode it than read it from memory

PROOF OF CONCEPT

Instances include:

CallyNft.sol

CallyNft.sol:241
CallyNft.sol:244
account is an address, so data.length == 20

TOOLS USED

Manual Analysis

MITIGATION

Replace

data.length

with

20