Cally contest - hyh's results

1. Wrong revert message (low)

Revert message is now misleading

Proof of Concept

Revert is performed (correctly) when dutchAuctionReserveStrike is too big, while the message states otherwise:

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

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

Recommended Mitigation Steps

Consider changing to:

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too big");

2. New operation initiating user facing functions miss emergency lever (non-critical)

If there be any emergency with system contracts, there is no way to temporary stop the operations.

Proof of Concept

The contract doesn't have pausing functionality for new operation initiation.

For example, createVault and buyOption cannot be temporary paused:

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

    /// @notice Creates a new vault that perpetually sells calls
    ///         on the underlying assets until a call option is exercised
    ///         or the owner initiates a withdrawal.
    /// @param tokenIdOrAmount The tokenId (NFT) or amount (ERC20) to vault
    /// @param token The address of the NFT or ERC20 contract to vault
    /// @param premiumIndex The index into the premiumOptions of each call that is sold
    /// @param durationDays The length/duration of each call that is sold in days
    /// @param dutchAuctionStartingStrikeIndex The index into the strikeOptions for the starting strike for each dutch auction
    /// @param dutchAuctionReserveStrike The reserve strike for each dutch auction
    /// @param tokenType The type of the underlying asset (NFT or ERC20)
    function createVault(

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

    /// @notice Buys an option from a vault at a fixed premium and variable strike
    ///         which is dependent on the dutch auction. Premium is credited to
    ///         vault beneficiary.
    /// @param vaultId The tokenId of the vault to buy the option from
    function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {

Recommended Mitigation Steps

Consider making all new actions linked user facing functions pausable, first of all createVault and buyOption.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

3. Exact strike msg.value can be cumbersome for users (non-critical)

It can be cumbersome for user to reproduce exact figure when manually dealing with the contract

Proof of Concept

Exercise now require exact strike to be sent over:

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

        // check correct ETH amount was sent to pay the strike
        require(msg.value == vault.currentStrike, "Incorrect ETH sent for strike");

Strike figure can have many meaningful digits as it's calculated via power law decline:

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

uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

Recommended Mitigation Steps

Consider allowing dust in msg.value (it will also align with premium logic):

        // check correct ETH amount was sent to pay the strike
        require(msg.value >= vault.currentStrike, "Incorrect ETH sent for strike");