Cally contest - Picodes's results

[Low - 01] buyOption should incorporate safeguards as the final strike price cannot be known in advance

Callers of buyOption cannot know the strike price they’ll end up getting. In most cases it is at their advantage: they just need to send their transaction when the strike price suits them and they’ll end up with a strike price of at most this value. However in a some cases it would be useful to have safeguards such as a deadline.

• deadline: imagine a pending transaction to buyOption that is executed during the next dutch auction period: without deadline, this can easily happen and lead to user paying the premium for a not chosen strike price. An other case would be to use the protocol in period of high prices volatility, then you may want to have calls to buyOption valid for only a short period of time to avoid paying for a useless option.

• maximumStrikePrice (way more extreme case): imagine a case where multiple participant compete to have their call to buyOption executed at a precise timestamp. They may send the transaction in advance so a maximumStrikePrice may be useful.

[Non-Critical - 01] Comment typo

Here: “which about 30% gas on buys/vault creations”, I assume a word is missing and it should be “which is about 30% gas on buys/vault creations”

[Non Critical - 02] Revert message typo

Here, the revert message is inverted: it should be “Reserve strike too big”

[Non-Critical - 03] Cally should mint NFTs starting by 1 or 0

Inverting this line and line 189 would enable minting of vaultID 1. Currently behavior is not intuitive as the first vault would be 3.

[Non-Critical - 04] Specify that _forceTransfer is intended to be used for minting as well

Here, the option will be minted if is not already existent. This behavior should be specified in the comments of _forceTransfer to make it clear that it’s the intended behavior.

[Non-Critical - 05] Requirements for msg.value are not coherent

In one place the code requires msg.value >= desired value, in the other msg.value == desired value. Both desired value are know in advance so it could unified, preferably to msg.value == desired value to protect user funds.

Reference: 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

[G - 01] Useless nonReentrant keyword

Here, in withdraw, the nonReentrant keyword is useless as the vault is burn line 335 before any external call, any reentrant call would fail because of the require line 323 as the owner would be the 0 address. Overall, ReentrancyGuard inheritance can be removed.

[G - 02] In CallyNFT, override transferFrom

Although the transfer functions of CallyNFT won’t revert because arithmetics are unchecked (see code), the contracts should still override transferFrom to remove the modifications of _balanceOf . First out of coherence and then for gas savings. You could also remove the _balanceOf mapping as it becomes useless. Note that this does not concern the Cally contract which does override transferFrom but only for other contracts that may use CallyNFT.