Cally contest - Kumpa's results

Lines of code

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

Vulnerability details

Impact

In the case that the owner's address is compromised or the owner rug pull, the owner could buyOption() from a vault and later exercise the option at a zero cost by updating the feeRate so that the fee can be as high as the currentstrike. ethBalance[getVaultBeneficiary(vaultId)] will equal to zero and the vault beneficiary will get nothing while his asset get stolen. This issue should be fixed to maintain the contract's reputation and trust from the user.

Proof of Concept

• The contract's owner initially set the protocol's feeRate at 1%
• Alice createVault() and deposits her nft
• The contract's owner buyOption from Alice's vault
• The contract's owner setFee() to 100% of the currentstrike
• The contract's owner exercise the option and send eth equal to vault.currentstrike
• With the adjusted feeRate, the fee will be equal to the currentstrike and ethBalance[getVaultBeneficiary(vaultId)] will be equal to zero

Recommended Mitigation Steps

• Set the maximum rate ceiling, or
• Fixed the feeRate that is unchangable to each vault when created.

Judge has assessed an item in Issue #119 as Medium risk. The relevant finding follows:

Checking whether the receiver is capable of holding ERC721 The contract usessafeTransfer() for ERC20 but uses transferFrom() for ERC721 in both exercise() and withdraw() which may lead to the loss of ERC721 if the receiving contract does not have onERC721Received(). To prevent this unintended circumstance, the contract should replace this function with safeTransferFrom() for ERC721 to check whether the receiving contract is IERC721Receiver.

1. Duration issue

The user may accidently set the durationDays to be max of uint8 (255 days) and has his asset locked up for a very long time if the option's buyer does not exercise the option. I think that it would be in the best interest of the user if the locked-up period is fixed to a reasonable period like 30 days and the contract's owner should cleary communicate about the risk of user's asset unintentionally getting locked up if the option is not exercised.

2. Two-step authorization in transferOwnership()

Transfering ownership should be two step processes. The contract relies on openzeppelin's ownable which maybe problematic. Even when the function checks for zero address, it overlooks the case that the current owner may accidently input wrong address. To add additional security layer, the contract should add the acceptOwnership() that the nominated account will have to call

3. Checking whether the receiver is capable of holding ERC721

The contract usessafeTransfer() for ERC20 but uses transferFrom() for ERC721 in both exercise() and withdraw() which may lead to the loss of ERC721 if the receiving contract does not have onERC721Received(). To prevent this unintended circumstance, the contract should replace this function with safeTransferFrom() for ERC721 to check whether the receiving contract is IERC721Receiver.