Cally contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

The setFee() function is used to set the protocol fee that is applied when a buyer exercises a call. This setter function does not apply any validation, meaning the feeRate_ parameter may be accidently or maliciously set to a value that leads to unintended consequences. Also, changes to the feeRate are applied to all future calls to exercise() which can be somewhat misleading to vault owners that deposited their ERC721/ERC20 while a lower fee was active.

Unwanted Result #1: The feeRate is set higher than 1e18, buyer will not be able to exercise the call.

If the feeRate is set to a value higher than 1e18, a call to exercise() will fail every time. This is because fee will be larger than msg.value via the following formula:

fee = (msg.value * feeRate) / 1e18;

Failing on underflow here:

ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

Motivation: This value could be accidently set too high or a malicious owner could intentionally set it too high in order to DOS the buyers from exercising calls. The potential motivation would be to let the calls expire to hopefully buy the call at a lower price.

Unwanted Result #2: Fee intentionally set higher than advertised right before exercise call.

Motivation: A malicious owner is able to advertise a low or 0 feeRate to attract users to deposit their ERC721s or ERC20s into Cally. At any point, the owner is able to set the feeRate higher. Most diabolically, the owner will set the feeRate higher right after a call is bought. At this point, the vault owner does not have any control over their deposited ERC721/ERC20. The owner increases the fee potentially as high as 100%, the call is exercised, all proceeds go to Cally.

This most likely is not intended from the Cally team, but users may view this possibility as a risk and choose not to deposit into Cally.

Tools Used

Manual review

Recommended Mitigation Steps

Firstly, I recommend adding thresholds for the min and max values that feeRate is able to be set within. This will provide validation against accidental or intended changes to feeRate that are not in the best interests of vault owners.

Secondly, changes to feeRate are applied immediately despite when the vault owner deposited their ERC721/ERC20. Perhaps it would be more fair to users if the same feeRate that was active during the deposit is stored in a mapping along with the vault id. This feeRate would be used until the call option has expired. This will allow the vault owner to withdraw their deposit if they don't agree to the new rate.

Lines of code

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

Vulnerability details

Impact

A griefer is able to create as many vaults as they want by simply calling createVault() with tokenIdOrAmount = 0. This will most likely pose problems on the front-end of the Cally protocol because there will be a ridiculously high number of malicious vaults displayed to actual users.

I define these vaults as malicious because it is possible that a user accidently buys a call on this vault which provides 0 value in return. Overall, the presence of zero-amount vaults is damaging to Cally's product image and functionality.

Proof of Concept

• User calls createVault(0,,,,); with an ERC20 type.
• There is no validation that amount > 0
• Function will complete successfully, granting the new vault NFT to the caller.
• Cally protocol is filled with unwanted 0 amount vaults.

Tools Used

Manual review

Recommended Mitigation Steps

Add the simple check require(tokenIdOrAmount > 0, "Amount must be greater than 0");

Lines of code

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

Vulnerability details

Impact

If a vault owner deposits an ERC20 token that supports fee on transfer, the vault owner will not be able to withdraw their deposit. Their funds will be frozen indefinitely until another deposit is made into Cally using the same ERC20.

Similarly, a call buyer will not be able to exercise their call on the ERC20 with fee on transfer until another deposit of the same token is made. If the call expires before more tokens are deposited, the buyer will permanently lose the funds used for their premium payment when buying the call.

This issue results in frozen funds, potential loss of premium, and lack of core protocol functionality.

Proof of Concept

For this example, assume the deposited ERC20 has a 1% fee on transfer.

• Vault owner deposits 10 ether of token.
• Cally receives (10 * 99%) = 9.9 ether
• No one buys the call.
• Vault owner attempts to withdraw their deposit.
• Cally attempts to send all 10 ether to vault owner. This fails because Cally only possesses 9.9 ether.
• Vault owners funds are frozen.

Exercising of calls fail because of similar logic:

• Vault owner deposits 10 ether of token.
• Cally receives (10 * 99%) = 9.9 ether
• Buyer buys call and attempts to exercise.
• Cally attempts to send all 10 ether to buyer. This fails because Cally only possesses 9.9 ether.
• Call expires. Buyer has lost the premium amount that they sent when buying the call.

It should be noted that the funds would be frozen until another deposit is made that brings Cally's balance higher than the balance needed by the vault.

Tools Used

Manual review.

Recommended Mitigation Steps

In the case of an ERC20 transfer, the vault tokenIdOrAmount should be written with the amount that was received by Cally after the transfer.

Take the balance of the ERC20 prior to and after the transfer. The difference will be the amount that is set for the vault.

[L-01] Comment vs Code conflict

Revert message indicates that reserve price is too small but condition checks that reserve is lower than the starting strike.

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

[L-02] Floating pragma

Abstract contracts contain a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions.

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/lib/solmate/src/tokens/ERC721.sol#L2

[L-03] Inconsistency regarding ETH transfer amounts

In buyOption(), the amount of ETH transferred can be greater than or equal to the premium amount. In 'exercise()`, the amount of ETH transferred must be EXACTLY the strike amount.

I recommend making the logic consistent to either be equal to or greater than or equal.

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

[L-04] Inconsistency in checking start/end time values

In the following snippet, the buyer must exercise their option BEFORE the expiration timestamp:

`require(block.timestamp < vault.currentExpiration, "Option has expired");

However, in this line, the vault owner is not able to withdraw until AFTER the expiration timestamp:

require(block.timestamp > vault.currentExpiration, "Option still active");

Neither of these checks allow for actions on the actual expiration timestamp. One of these should be inclusive of the expiration time.

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

[N-01] - Inconsistency in checking bool values

Throughout Cally.sol, bools are checked via == true, == false, bool, and !bool. The below snippet is a single example:

        require(vault.isExercised == false, "Vault already exercised");
        require(vault.isWithdrawing, "Vault not in withdrawable state");

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

I recommend sticking to a single form of checking bool values.

[O-01] Move vault read operation below checks that vault actually exists

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

[O-02] Define the length of the arrays premiumOptions and strikeOptions

Since these arrays do not have setters to change their length, define their length when defining them. In my quick testing:

Reading a value from current implemented premiumOptions: 26052 gas

Reading value from premiumOptions with defined length: 23901 gas