Cally contest - hubble's results

Lines of code

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

Vulnerability details

The vulnerability or bug is in the implementation of the function getDutchAuctionStrike() The AUCTION_DURATION is defined as 24 hours, and consider that the dutchAuctionReserveStrike (or reserveStrike) will never be set to 0 by user.

Now if a vault is created with startingStrike value of 55 and reserveStrike of 13.5 , the auction price will drop from 55 to 13.5 midway at ~12 hours. So, after 12 hours from start of auction, the rate will be constant at reserveStrike of 13.5, and remaining time of 12 hours of auction is a waste.

Some other examples :

startStrike, reserveStrike, time-to-reach-reserveStrike
55 , 13.5  , ~12 hours
55 , 5     , ~16.7 hours
55 , 1.5   , ~20 hours
5  , 1.5   , ~11 hours

Impact

The impact is high wrt Usability, where users have reduced available time to participate in the auction (when price is expected to change). The vault-Creators or the option-Buyers may or may not be aware of this inefficiency, i.e., how much effective time is available for auction.

Proof of Concept

Contract : Cally.sol Function : getDutchAuctionStrike ()

Recommended Mitigation Steps

The function getDutchAuctionStrike() can be modified such that price drops to the reserveStrike exactly at 24 hours from start of auction.

        /*
            delta = max(auctionEnd - currentTimestamp, 0)
            progress = delta / auctionDuration
            auctionStrike = progress^2 * (startingStrike - reserveStrike)             << Changes here
            strike = auctionStrike + reserveStrike                                    << Changes here
        */
        uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;
        uint256 progress = (1e18 * delta) / AUCTION_DURATION;
        uint256 auctionStrike = (progress * progress * (startingStrike-reserveStrike)) / (1e18 * 1e18);

        strike = auctionStrike + reserveStrike;

Lines of code

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

Vulnerability details

When a vault is created, the creator knows the value of feeRate that will be charged. But, after the option is sold, there is a possibility of changing the feeRate to a high value by the contract Owner. In the event the exercise() is called, the beneficiary will get a lower value due to higher protocol fee.

Impact

This is a known risk, and users may not trust the protocol due to this control the contract Owner has. Vault beneficiary can also be grieved by maliciously configuring a high feeRate, before the option is exercised. The impact will be on all the protocol users who's options will be eligibile for exercise. This is also amplified as currently there is no upper bound check on the feeRate that can be set.

Any new feeRate should be applicable only to newer vaults created henceforth.

Proof of Concept

Contract : Cally.sol Function : setFee() and exercise()

Recommended Mitigation Steps

In the Vault struct, add another parameter feeRate, and during createVault, set it to the current value of feeRate. In the event of exercise() function being called, the stored value of feeRate in the Vault struct to be used as opposed to the current global feeRate.

Lines of code

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

Vulnerability details

There is no whitelist or check on the ERC20 tokens to be used during Vault creation. So, if a feeOnTransfer ERC20 is used as token during vaultCreation, then actual amount transferred into the contract will be less than the vault.tokenIdOrAmount

Assuming only one vault is created for this specific type of token currently, and an option is sold, then if the option is exercised, the actual balance of the feeOnTransfer token will be less than the original vault.tokenIdOrAmount, hence exercise will fail on safeTransfer due to insufficient balance. Alternately if the option has expired, and the vault owner wants to withdraw the token, it will fail similarly due to insufficient balance.

Impact

Option cant be exercised by option holder or token withdrawn by vault owner, unless manually the difference amount of token is transferred to the contract by somebody. The difference amount is the feeOnTransfer value deducted during vault creation.

Recommended Mitigation Steps

Either the actual amount transferred into the contract is stored in the vault.tokenIdOrAmount, or the vault creator adds the diff of the feeOnTransfer amount also, while creating the vault.

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;