Cally contest - Bludya's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L117-L121 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282-L286

Vulnerability details

Impact

From an user standpoint, if I create a vault, I expect to have the protocol fee set in the time of creation. Before creating it, I would've calculated the value of the option based on the momentary protocol fees, among other things. In the current implementation, the protocol fee percentage can be changed at any time. Not only that, but it also has no upper limit. The fee amount is calculated in the time of the option exercising, based on the fee percentage and the currentStrike.

Those three facts lead to the following two expected issues:

1. Every time the fee percentage is changed, all vaults, created before the fee percentage change, will have different amount of fee at the time of the exercise of the option.
2. More importantly, the following case is possible - user Val creates a vault. The contract owner then buys the option for that vault and then sets the protocol fee to 100%. Now when the contract owner exercises the option, the whole strike amount will be set as protocol fee. In that case Val will receive nothing and the contract owner will have both the asset and the paid strike.

Proof of Concept

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L117-L121 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282-L286

Tools Used

none

Recommended Mitigation Steps

Add a propererty in the vault struct, that holds the fee percentage at the time of its creation and calculate the fee based on that.

Summary statement

The code is relatively well written, well documented and readable. There isn't much possibility for attacks, since the external function calls are either in a nonreentrant function or properly placed so that a reentry can't do any harm. Also not having many calculations based on dynamic, external, chain values helps.

One concerning low risk problem is that the protocol fee is calculated in the exercise function which might be unexpected from an user standpoint and is not documented. More info on this can be found down in the issue explanation.

Dictionary

NC-<number> - non-critical issue L-<number> - low risk issue

NC-1 Some functions don't seem to need to be public

Based on what they do and how they are used in the source code I see, some functions don't need to be public and currently are:

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L51 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L72 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L236 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L455 - can be external

Solution

Make the mentioned functions private, internal, external depending on the use.

NC-2 Some of the state variables can be immutable or constants

Setting some state variables are only set at compile and not changed at all later, so they coult be set to constants. This would also save some gass when getting their values.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L90 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L92 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L92

Solution

Set a constant modifier to the mentioned variables.

NC-3 Little inconsistancy in currentExpiration checks.

In buyOption the require, checking if the auction has started checks if the block timestamp is >= than the currentExpiration. In the withdraw function the same check is done with >. There seems to be no reason for the two checks to be different, so for consistency sake they could be made the same.

Occurance

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

L-1 Wrong usage of getPremium

In tokenUri, when calling renderJson, the function getPremium is used wrongly. It is being called with the premiumIndex, rather than the vaultId which it actually expects. This makes it also return totally wrong value.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L464

Solution

Also replace getPremium(vault.premiumIndex) with premiumOptions[vault.premiumIndex] in Cally.sol line 464.

G-1 Incrementing with prefix rather than suffix ++ is cheaper.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L244

Solution

Replace example++ with ++example where possible ( i++ with ++i).

G-2 Get <array>.length outside of loop

When array.length is looked up in the loop construct the following gas overheads are incured every time the loop turns

storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset"

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L244

Solution

Get the array length in a variable before the loop and then use that variable in the loop.

G-3 Where overflow/underflow can't occur for sure, unchecked() can be used

Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, which includes additional checks, which incur more gas. To use the old behaviour without them, the operation can be wrapped in unchecked().

Occurance

Places where I see this can be done are these: https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L244

Since data will always be the same length it can be used for the bytes innitialisation here as well: https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L241

And for the index calculation here: https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L245 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L246

Solution

Example before:

        for (uint256 i = 0; i < data.length; i++) {
            //something
        }

Example after:

        for (uint256 i = 0; i < data.length) {
            //something
            unchecked(i++);
        }

G-4 There is no need to set zero values where it is such by default, this incurs a bit more gas

When initializing uint, int etc, the default value is 0. Setting it to 0 again incurs more gas.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/CallyNft.sol#L244 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282

These will incur only on deployment and are negligible, but still: https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L94 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L95

Solution

Remove the value setting in all the places where it is used to initialize a variable with its default value.

G-5 There is no reentrancy attack vector in function withdraw, so nonReentrant can be removed

Reentrancy is a problem when the state is not updated before another contract function is called, which allows a malicious contract to reenter the function it was called from and redo it many times and maybe withdraw its funds or do some other harm. In the withdraw function you do all the withdraws last and before them you burn the vault and option. Since the function has a check if the vault is burned, an attacker would not be able to reenter the function with the same vaultId. Since reentrancy can't harm the function anyways, there is no need for the nonReentrant modifier, which incurs a lot of gas. 3500~ gas per call could be saved.

Occurance

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

Solution

Remove the nonReentrant modifier.

G-6 Replace > 0 when comparing unsigned integers

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements. You could replace them to save some gas.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L170 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L283

Solution

Replace > 0 with != 0 in the mentioned cases.

G-7 Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L80 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L81

Solution

Use uint256(1) and uint256(2) for true/false

G-8 Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L76 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L77 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L78

Solution

Use uint32/int32 and downcast if needed

G-9 Duplicated require()/revert() checks should be refactored to a modifier or function

This would save deployment gas

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L211 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L304 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L320 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L353

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L307 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L323 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L354

Solution

Put the checks in function modifiers and use them where needed.

G-10 Value getting should be done after all posible checks

If checks are done no matter what and can be done using only the call variables, they should be done before any value setting/getting so that that gas is not spent when the function will revert.

Occurance

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

Solution

Move the checks before the value getting where possible.

G-11 Functions guaranteed to revert when called by normal users can be marked payable

If a function is guaranteed to revert on normal user call, for example with modifier onlyOwner, then it could be set to payable since there is no risk of user losing funds. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Occurance

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

Solution

Set the mentioned functions with payable modifier.

G-12 If a variable is read only once, not caching it can save gas.

This could make the code less readable, but on some places it might be worth it since it saves some gas.

Occurance

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L223 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L227 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L249 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L333

Solution

Remove the caching and get the value directly where it is used.