QA Report

[code423n4]

Summary of Findings for Low / Non-Critical issues

LOW

• L-01 : wrong error message string in function createVault()
• L-02 : function vaults() can return misleading information
• L-03 : initiateWithdraw() should not be callable , if option already exercised
• L-04 : ambiguous error message in createVault() for durationDays
• L-05 : the available values in premiumOptions[] & strikeOptions[] are too restrictive
• L-06 : No event is raised when feeRate is changed
• L-07 : No event is raised when vault beneficiary is changed

NON-CRITICAL

• NC-01 : consistency in fetching vault values
• NC-02 : valutIndex = 1 is never used

Details L-01

Title : wrong error message string in function createVault()

Function : createVault() in Cally.sol

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

Impact

If the current error message is followed, user will never be able to successfully createVault()

Recommended Mitigation Steps

Correct error message string

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike more than Starting strike");

Details L-02

Title : function vaults() can return misleading information

VaultId are odd in number; if a valultId of event number is given, the function valuts() will return misleading information.

Function valuts() in Cally.sol

Recommended Mitigation Steps

Check if the valutID parameter is of vault type, by adding a require statement

function vaults(uint256 vaultId) external view returns (Vault memory) {
    require(vaultId % 2 != 0, "Not vault type");
    return _vaults[vaultId];
}

Details L-03

Title : initiateWithdraw() should not be callable , if option already exercised

If the option is already exercised, the vault owner should not be allowed to call the initiateWithdraw() function.

Recommended Mitigation Steps

Add a require statement in the function initiateWithdraw()

require(vault.isExercised == false, "Vault already exercised");

Details L-04

Title : ambiguous error message in createVault() for durationDays

If a value of 0 is given for durationDays in the createVault() function, the transaction will revert with an ambigous message "durationDays too small"
It can better stated as given below

Recommended Mitigation Steps

Error message string can be changed as below.

line 170 require(durationDays > 0, "durationDays cannot be zero");

Details L-05

Title : the available values in premiumOptions[] & strikeOptions[] are too restrictive

To reduce gas and storage, the protocol has currently designed to store the index of the premiumOptions[] & strikeOptions[] in the Vault structure.

Impact

This is too restrictive and may not be future proof.

Recommended Mitigation Steps

One suggestion is to add another member unit8 premiumMultiplier (with default value of 1) in the Vault struct, and users can have combination of values of the
premiumMultiplier and premiumIndex to define more range of premium values if required.

Same suggestion applies for adding a multiplier for the strikeOptions[]

Details L-06

Title : No event is raised when feeRate is changed

Impact

When feeRate is changed at setFee(Cally.sol), no event is raised. It would be important to raise this event for any external integration with this system.

Proof of Concept

Contract : Cally.sol
Function : setFee

Recommended Mitigation Steps

event definition

event FeeRateUpdated(uint256 newFeeRate);

event emit at setFee function

require(feeRate_ != feeRate, "new feeRate should be different");
feeRate = feeRate_;
emit FeeRateUpdated(feeRate_);

Details L-07

Title : No event is raised when vault beneficiary is changed

Impact

When beneficiary is changed at setVaultBeneficiary(Cally.sol), no event is raised. It would be important to raise this event for any external integration with this system.

Proof of Concept

Contract : Cally.sol
Function : setVaultBeneficiary

Recommended Mitigation Steps

event definition

event VaultBeneficiaryUpdated(uint256 indexed vaultId, address indexed beneficiary);

event emit at setVaultBeneficiary function

emit VaultBeneficiaryUpdated(vaultId, beneficiary);

Details NC-01

Title : consistency in fetching vault values

In Cally.sol, function buyOption the following is the order of lines.
line 208 require(vaultId % 2 != 0, "Not vault type");
line 211 Vault memory vault = _vaults[vaultId];

This can be made consistent with other functions by changing the order.

Vault memory vault = _vaults[vaultId];
require(vaultId % 2 != 0, "Not vault type");

Details NC-02

Title : valutIndex = 1 is never used

The value of vaultIndex = 1 is never assigned to any vault.

        // vault index should always be odd
        vaultIndex += 2;
        vaultId = vaultIndex;
        _vaults[vaultId] = vault;

This can be changed to as below, so that vaultID = 1 is also used

        vaultId = vaultIndex;
        // vault index should always be odd
        vaultIndex += 2;
        _vaults[vaultId] = vault;

[outdoteth]

high quality report

[HardlyDifficult]

I agree with the low/non-critical labeling provided by the warden in this report.