search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

QA Report #268

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the lack of checks in setters.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol: 195 emit NewVault(vaultId, msg.sender, token);//emitted before tokens transferred to contract.
Cally.sol: 291 emit ExercisedOption(optionId, msg.sender);//emitted before tokens transferred to user.
Cally.sol: 337 emit Withdrawal(vaultId, msg.sender);//emitted before harvest() call and tokens transferred to user.
Cally.sol: 365 emit Harvested(msg.sender, amount);//emitted before ETH transferred to user.

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:119: setFee(uint256 feeRate_)
Cally.sol:351: setVaultBeneficiary(uint256 vaultId, address beneficiary)

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:455: function tokenURI(uint256 tokenId)

CallyNft.sol

CallyNft.sol:51: function renderJSON()
CallySvg.sol:136: function renderSvg()
CallyNft.sol:236: function addressToString()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:103: mapping(uint256 => Vault) private _vaults;
Cally.sol:108: mapping(uint256 => address) private _vaultBeneficiaries;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, for the Cally.sol mappings, the mitigation could be:

struct VaultInformation {
  Vault _vault;
  address beneficiary;
}

And it would be used as a state variable:

mapping(uint256 =>  VaultInformation) private vaultInformation;

nonReentrant modifier unused

PROBLEM

Some external functions calling the ERC20 methods safeTransfer or safeTransferFrom do not have the nonReentrant modifier and are hence unprotected to reentrancy (besides the gas limit on the methods). No funds are directly at loss but it is best practice to avoid reentrancy altogether.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:158: function createVault()
Cally.sol:258: function exercise()
Cally.sol:368: function harvest()

TOOLS USED

Manual Analysis

MITIGATION

Use the nonReentrant modifier on these functions.

High feeRate can break core protocol function

PROBLEM

There is no maximum input value on setFee() in Cally.sol. But if the owner sets it to a uint greater than 1e18, the users will not be able to call exercice() as the function will revert, breaking the protocol's functionality.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:284: fee = (msg.value * feeRate) / 1e18;

If feeRate is set so that ethBalance[getVaultBeneficiary(vaultId)] + msg.value < fee, and the following statement will revert

Cally.sol:289: ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

TOOLS USED

Manual Analysis

MITIGATION

Add a check in setFee to ensure the new fee rate is less than a maximum maxFeeRate. Its value depends on different factors, but considering it determines how much ETH a vault creator is receiving from a strike, it should be reasonably low (ie less than 0.5 * 1e18)

Unchecked inputs

PROBLEM

Setters should check the input value - ie make revert if it is the zero address. Here, if the vault beneficiary is set as the zero address, all the strike ETH associated with the vault will be locked.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:351: setVaultBeneficiary()

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check

outdoteth commented 2 years ago

this can be bumped to medium severity: High feeRate can break core protocol function; https://github.com/code-423n4/2022-05-cally-findings/issues/48

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-05-cally-findings/issues/271

HardlyDifficult commented 2 years ago

Moved High feeRate to https://github.com/code-423n4/2022-05-cally-findings/issues/325