Cally contest - catchup'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#L283-L285

Vulnerability details

Impact

Admin can change the protocol fee anytime, to any value by calling setFee() function. Even if there is no bad intention, this can potentially be used to damage the reputation of the protocol.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual analysis

Recommended Mitigation Steps

• A timelock can be added for such critical changes which may effect user decisions.
• A hardcoded max fee limit can be defined and setFee() can check the new fee against max fee.

Lines of code

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

Vulnerability details

Impact

exercise() function is called by a call option buyer to exercise the option. When this happens below code transfers the underlying NFT to the exerciser: // transfer the NFTs or ERC20s to the exerciser vault.tokenType == TokenType.ERC721 ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount) : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount); However, if the receiver address is a contract address which do not have ERC721 support, then the NFT will be frozen in the contract.

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

I suggest to use safeTransferFrom, so that if the receiver can not receive the NFT, exercise would revert. // transfer the NFTs or ERC20s to the exerciser vault.tokenType == TokenType.ERC721 ? ERC721(vault.token).safeTransferFrom(address(this), msg.sender, vault.tokenIdOrAmount) : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

feeRate can be initialized using a real life value

feeRate is initialized as 0 which can be updated using setFee() function. It is commented in the setFee() function that the fee will be 1%. Considering 1% is the default fee, it would be better to initialise the feeRate as 1% rather than 0. In case setFee() is not called or called late by the admin for some reason, protocol fee collection would not be interrupted.

Lines of code

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

An idea to handle premiumOptions and strikeOptions

It is explained in the documentation that vault information is represented as a packed struct to save gas. I believe premiumIndex and dutchAuctionStartingStrikeIndex can both be made uint16 without requiring an extra slot: address token; // 20 bytes uint16 premiumIndex; // 2 bytes uint8 durationDays; // 1 byte uint16 dutchAuctionStartingStrikeIndex; // 2 bytes uint32 currentExpiration; // 4 bytes bool isExercised; // 1 byte bool isWithdrawing; // 1 byte total : 31 bytes Then premiumOptions and strikeOptions look-up tables can be completely deleted (saving gas). Rather than using look-up tables, premium can be calculated by scaling the premiumIndex value using a max limit. For example:

uint256 private constant premiumMax = 100 ether;

function getPremium(uint256 vaultId) public view returns (uint256 premium) {
	Vault memory vault = _vaults[vaultId];
	uint256 premium = vault.premiumIndex * premiumMax / type(uint16).max
	return premium;
}

I haven't tested the above code. I've just scrabbled it to explain the idea. If non-zero premium is needed, we need to have some controls to prevent it. With this approach a very high resolution can be achieved for premium and strike values with probably lower gas costs.

Lines of code

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#L394-396

Important changes should emit events

Functions which are executed only by privileged users and change important parameters should emit events. I suggest to include events for the below functions.

Lines of code

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

Missing non-zero address check

It is a good practice to include non-zero address check for important addresses. I suggest to include a non-zero address check for the token address in createVault()

Lines of code

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

Better to check msg.value with >=

msg.value is checked to be exactly same as currentStrike in exercise() function: // check correct ETH amount was sent to pay the strike require(msg.value == vault.currentStrike, "Incorrect ETH sent for strike"); It might be better to check with >= , as it was done for premium within buyOption() function: // check enough eth was sent to cover premium uint256 premium = getPremium(vaultId); require(msg.value >= premium, "Incorrect ETH amount sent");

Lines of code

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

for loops can be optimized

1- For loop index increments can be made unchecked. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Also postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

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

Recommended Mitigation Steps

I suggest to change the original code from this for (uint256 i = 0; i < data.length; i++) { str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))]; str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))]; } to this for (uint256 i; i < data.length; ) { str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))]; str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))]; unchecked { ++i; } }

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

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 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L126 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L180 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L181 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L183 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282

Using != 0 is cheaper than > 0 when used on a uint in a require() statement

Lines of code

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

Public state variables can be private

Private state variables are cheaper than public state variables. Below instances can be private.

Lines of code

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#L94 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L95 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L100 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L111

Non-zero amount check missing before transfers

It is a good practice to apply non-zero amount checks for transfers to avoid unnecessary executions.

Lines of code

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

Public constant can be made private

AUCTION_DURATION is only used within the contract it is defined. Therefore, it is unnecessary to define it as public.

Lines of code

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

feeRate can be cached and used

feeRate within exercise() function is accessed twice. It can be cached in a local variable to save an SLOAD.

Lines of code

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

auctionStartTimestamp can be deleted

auctionStartTimestamp can be deleted. This original code: // check option associated with the vault has expired uint32 auctionStartTimestamp = vault.currentExpiration; require(block.timestamp >= auctionStartTimestamp, "Auction not started"); Can be changed as: // check option associated with the vault has expired require(block.timestamp >= vault.currentExpiration, "Auction not started");

Lines of code

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

Booleans are more expensive than uint256

It is more expensive to operate using booleans because before every write operation an SLOAD is executed to read the contents of the slot. Therefore, it is cheaper to use uint256 instead of bool. On the other hand, using bool is better for the code readability. Hence, it is a tradeoff to be decided by the developers.

Lines of code

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

It is better to use 1-2 for isExercised and isWithdrawing rather than 0-1

SSTORE from 1 to 2 is cheaper than SSTORE from 0 to 1. https://github.com/code-423n4/2022-01-yield-findings/issues/102

Lines of code

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