Cally contest - FSchmoede's results

Lines of code

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

Vulnerability details

Missing check for setFee method

In the contract Cally.sol there is no check/upper limit for what the feeRate can be set with the setFee() method. It can be changed to a unintended high number either by a mistake or by a malicious owner.

Impact

This issue have 2 different scenarios:

Scenario 1)

The owner(s) of the contract could potentially set the fee to 100% ((100 * 1e18) / 100) which would make the user loose all their earned strike. This can scare some users away, as they will never be sure if the fee is increased to a high percentage after they created their vault, and hence will loose both the strike and NFT/tokens if an option is exercised.

Scenario 2)

If the fee it set to a value higher than 100% e.g. 105% ((105 * 1e18) / 100), then the method exercise() will cause an underflow when the new ethBalance is calculated. This will cause the method to revert every time, and hence to options will be able to be exercised. This can impact the users who bought an option call, because they might not be able to exercise the option, when there is a change of making profit.

Proof of concept

Scenario 1)

1. Alice creates vault with 0.1 ETH premium, 30 day duration on her BAYC, max strike of 500 ETH

2. A malicious/clumsy owner sets the feeRate to 100%

3. Bob buys the call for 0.1 ETH, 30 day duration at a strike of 164.3 ETH after T amount of time has passed

4. Bob exercises his option after 23 days

5. He sends 164.3 ETH (strike amount) to the contract and the BAYC is sent to him. Most of the 164.3 ETH (considering a normal fee rate) was supposed to go to Alice, but since the feeRate is 100% then Alice’s ethBalance will not increase. This means Alice lost her BAYC and ~164.3ETH she was supposed to receive in return of the BYAC.

The calculation of the fee amount upon exercise can be seen here L281-L289

// collect protocol fee
uint256 fee = 0;
if (feeRate > 0) {
    fee = (msg.value * feeRate) / 1e18; // 164.3 * (100 * 1e18) / 100 = 164.3
    protocolUnclaimedFees += fee;
}

// increment vault beneficiary's ETH balance
ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee; // 164.3 - 164.3 = 0

Scenario 2)

1. Alice creates vault with 0.1 ETH premium, 30 day duration on her BAYC, max strike of 500 ETH

2. A malicious/clumsy owner sets the feeRate to 105%

3. Bob buys the call for 0.1 ETH, 30 day duration at a strike of 164.3 ETH after T amount of time has passed

4. Bob exercises his option after 23 days, but it reverts because the calculation of ethBalance will underflow making it impossible for Bob to exercise the option.

The calculation of the increase of ethBalance upon exercise can be seen here L281-L289

// collect protocol fee
uint256 fee = 0;
if (feeRate > 0) {
    fee = (msg.value * feeRate) / 1e18; // 164.3 * (105 * 1e18) / 100 = 172.515
    protocolUnclaimedFees += fee;
}

// increment vault beneficiary's ETH balance
ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee; // 164.3 - 172.515 = -8.215 (hence msg.value will underflow)

Recommended mitigation

Add a check in the method seeFee() that ensures that the new fee cannot exceed some value, e.g. 20%. The method would then look like this:

function setFee(uint256 feeRate_) external onlyOwner {
    require(feeRate_ <= ((20/100) * 1e18), "FeeRate exceeds 20 percent");
    feeRate = feeRate_;
}

Lines of code

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

Vulnerability details

Use safeTransferFrom for ERC721

In the contract Cally.sol every transfer of ERC721 are done with the transferFrom() instead of the recommended safeTransferFrom(). This transferFrom() does not check, whether the receiver is capable of proper handling of NFTs.

Impact

If the buyers of a call option with a ERC721 collateral is a smart contract (could be a multisig wallet) which does not implement the onERC721Received and do not have proper handling of ERC721 tokens, then the NFT would be lost if the option is exercised.

Proof of concept

1. Alice creates a vault with a BAYC NFT

2. SomeDAO choose to buy the call option from their multisig wallet

3. SomeDAO exercises the option when there is a change of making a profit

4. The BAYC NFT is transferred to SomeDAO’s multisig wallet, but by a mistake the wallet does not support handling of ERC721 tokens.

5. SomeDAO have no way of retrieving the BAYC NFT, and has hence paid for a NFT they cannot get.

Even though the scenario is unlikely, and it would be the users own fault, there is no doubt that when it requires so small an effort, it makes sense to make it impossible to do this rooky mistake.

A similar issue was also scored as a medium see here.

The relevant code:

L199, L295, and L344.

Recommended mitigation

It is recommended to use the safe version safeTransferFrom() since this will require the receiving contract to implement IERC721Receiver.onERC721Received

QA report

[N-01] Consistent naming for getter functions

There are 4 getter functions in the Cally.sol contract, where 3 of them are prefixed with get. The last one to get vault information is just called vaults().

Consider renaming it to getVault() or getVaultInformation() to add consistency and clarity for getter functions.

The vaults() function can be found here: L387-389

[N-02] Missing event for setVaultBeneficiary

All other functions but setVaultBeneficiary that change information about a vault will emit a relevant event. Consider emitting a VaultBeneficiaryChanged event to add consistency and also inform any relevant listeners waiting for this change to happen.

The function can be found at L351-L357

[N-03] Be consistent in named returns

3 out of 4 getter functions in Cally.soluse named return variables, where two of them getVaultBeneficiary() and getPremium() doesn’t utilize it. Add consistency by either use named returns for all or none of the getter methods. getVaultBeneficiary(), vaults() and getPremium() can be rewritten into to use named returns:

L378-L383

function getVaultBeneficiary(uint256 vaultId) public view returns (address beneficiary) {
    address currentBeneficiary = _vaultBeneficiaries[vaultId];
    
     // return the current owner if vault beneficiary is not set
    beneficiary = currentBeneficiary == address(0) ? ownerOf(vaultId) : currentBeneficiary;
}

L387-L389

function vaults(uint256 vaultId) external view returns (Vault memory vault) {
    vault = _vaults[vaultId];
}

L394-L397

function getPremium(uint256 vaultId) public view returns (uint256 premium) {
    Vault memory vault = _vaults[vaultId];
    premium = premiumOptions[vault.premiumIndex];
}

Gas optimization report

[G-01] Redundant initialization of variables

In Cally.sol there are 2 variables that are initialized with the value 0. This is redundant and a waste of gas since the variables automatically will be assigned the value 0 if they are declared without a value.

Code: L94-L95

uint256 public feeRate = 0;
uint256 public protocolUnclaimedFees = 0;

Can be changed into:

uint256 public feeRate;
uint256 public protocolUnclaimedFees;

This will, according to the Foundry gas report, save 4412 gas on deployment of the contract.