Cally contest - antonttc's results

Lines of code

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

Vulnerability details

Impact

A malicious admin can set feeRate to 100% and take all the money paid in exercises.

Proof of Concept

Not exactly an "exploit" to the code, but having no limitation on this parameter will raise centralization risk concern among users. Also in case of a key lost, having a hard cap like 10% can make sure even the admin key is compromised, no user funds will be at risk.

Mitigation steps

function setFee(uint128 feeRate_) external onlyOwner {
        require(feeRate_ < 1e17, "illegal");
        feeRate = feeRate_;
    }

Lines of code

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

Vulnerability details

Impact

NFT can get trapped in user contract because of some mistakes.

Proof of Concept

In exercise function, when the contract is transferring NFT to user contract, it use transferFrom instead of safeTransferFrom which skips the check of receiver contract is willing to receive NFT. This might lead to NFT permanently locked in a contract.

There has been debates about whether to use transferFrom or safeTransferFrom, but I personally think it's always better to use safeTransferFrom: if the user use a contract wallet without the receiving callback, he can always transfer the optionToken to another EOA address and exercise; But, if there's a mistake and a contract accidentally receive and NFT, it will get locked forever.

Recommended Mitigation Steps

consider using safeTransferFrom (and safeMint), otherwise explicitly state it somewhere in the code or document that it doesn't fully complied with ERC721.

Lines of code

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

Vulnerability details

Impact

In the createVault function, the vaultType is left for the user to specify. If a user accidentally specify a wrong type, the asset will still be pull because of the interface of transferFrom for ERC20 and ERC721 is the same, but the asset will be locked forever in the contract because no one can withdraw them.

Proof of Concept

• a user creates a vault with bayc address, but accidentally specify tokenType to TokenType.ERC20.
• during withdraw or exercise while transferring out ERC20, all calls will revert because there's no .transfer(to, amount) on bayc. So it reverts with "TRANSFER_FAILED".

Recommended Mitigation Steps

Consider adding a custom mapping of address => {whitelisted, tokenType} and maintain a list of tokens the protocol want to support.

Another attack factor from letting people specify token address and tokenType is that people can create malicious options with malicious ERC20 tokens that cannot be exercise, so I believe it's better that the team controls what can be created out of the protocol.

Low risk findings:

1. tokenURI pass in the wrong parameter for getPremium

It should accept a vaultId, but here the code pass in premiumId

getPremium(vault.premiumIndex),

This will make the NFT showing the wrong trait. P.S. I suggested changing the input parameter of getPremium from vaultId to premiumIndex, as described below (gas optimization #2)

2. wrong revert message in createVault

This revert message:

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");

should instead be reserve strike too high.

Gas optimizations

1. pack variables feeRate and protocolUnclaimedFees together to save gas on exercise.

Because these 2 variables are used together in exercise function, (reading feeRate and updating protocolUnclaimedFees), packing them into the same storage slot will save you one more SLOAD if feeRate > 0. on average this saves around 2000 gas. (49k -> 47K)

2. change getPremium input to premiumIndex.

As it currently is, getPremium function receive vaultId read the vault from storage again and than get the premiumIndex. This will cause an extra SLOAD to load the vault from storage. This can be avoided if we just pass in premiumIndex, as all 2 functions that call this already have their vault instance in memory. This makes average cost of buyOption drops from 75.7K -> 74.8.

3. pack currentStrike and dutchAuctionReserveStrike into a single storage slot.

Since packing variables is highly used in the vault struct, i suggest also pack dutchAuctionReserveStrike and currentStrike to uint128 to save 1 SLOAD and SSTORE from read and write. The gas saving from running the benchmark is 74.8K -> 73.5K

Gas optimizations

1. pack variables feeRate and protocolUnclaimedFees together to save gas on exercise.

Because these 2 variables are used together in exercise function, (reading feeRate and updating protocolUnclaimedFees), packing them into the same storage slot will save you one more SLOAD if feeRate > 0. on average this saves around 2000 gas. (49k -> 47K)