QA Report

[code423n4]

Low Risk Issues

Vaults can be created with non-existent/destructed tokens

At the top of SafeTransferLib.sol is the following comment:

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

The functions safeTransferFrom() and safeTransfer() from SafeTransferLib.sol do not check if a token contract actually contains code. Thus, if the provided token address has no code, these functions will not revert as low-level calls to non-contracts always return success.

For example, if a user creates a vault and provides address(0) as the token address, functions such as withdraw() and exercise() will not revert despite the use of a non-legitimate token address.

Below are the instances where ERC20 token transfers are made without checking for code existence:

Cally.sol:200           : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
Cally.sol:296           : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
Cally.sol:345           : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

To prevent users from creating vaults with non-legitimate token addresses, consider verifying the token parameter in createVault() actually contains code. For example:

    require(token.code.length > 0, "token is not contract")

Recommend using safeTransferFrom() instead of transferFrom() for NFTs

The EIP-721 standard states:

    /// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
    ///  TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE
    ///  THEY MAY BE PERMANENTLY LOST

Cally.sol uses transferFrom() to facillitate the transfer of ERC721 NFTs from the contract to users, as shown below.

In exercise():

Cally.sol:295          ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)

In withdraw():

Cally.sol:344          ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)

To prevent a permanent loss of NFTs, there should be checks in place to ensure msg.sender is capable of receiving NFTs. Otherwise, consider using safeTransferFrom() instead of transferFrom().

Misleading comment on transferFrom() in Cally.sol

The comment below implies the vault beneficiary is explicitly set to the account receiving the NFT, when it is merely set to address(0):

Cally.sol:430           ///      The new beneficiary will be the account receiving the vault NFT.

I suggest removing this line to prevent confusion.

Incorrect require string in createVault()

Should be "Reserve strike too big" in Cally.sol:169:

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

Missing parameter validation in functions getVaultBeneficiary(), vaults(), getPremium()

These functions are only meant to be provided with IDs corresponding to vaults. To prevent confusion, they should check if the provided vaultId is valid, and revert should users accidentally provide an optionId instead.

I recommend adding the following check to getVaultBeneficiary(), vaults(), getPremium():

require(vaultId % 2 != 0, "Not vault type");    

Non-Critical Issues

Constants should be used rather than magic numbers

Instead of using 1e18, I suggest using 1 ether to improve code readability.

• Cally.sol:284:

    fee = (msg.value * feeRate) / 1e18;

• Cally.sol:418-419:

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

Typos

it's should be its:

Cally.sol:315           ///         unharvested premiums for the owner. Vault and it's associated option

vaultId's should be vaultId

Cally.sol:352           // vaultId's should always be odd

string.concat() can be used instead

string.concat() can be used instead of abi.encodePacked(<str>,<str>) in Cally.sol:473:

    return string(abi.encodePacked("data:application/json;base64,", Base64.encode(bytes(jsonStr))));

Unecessary naming of return variables

Declaring a name for return variables in the following functions serve no purpose. I suggest only declaring the variable type.

• Cally.sol:378-383:

    // Can be changed to: function ... returns (address) {
    function getVaultBeneficiary(uint256 vaultId) public view returns (address beneficiary) { 
        // ...
        return currentBeneficiary == address(0) ? ownerOf(vaultId) : currentBeneficiary;
    }

• Cally.sol:394-397:

    // Can be changed to: function ... returns (uint256) {
    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        // ...
        return premiumOptions[vault.premiumIndex];
    }

[outdoteth]

this can be bumped to high severity:

Vaults can be created with non-existent/destructed tokens: #225

and this can be bumped to medium severity:

Recommend using safeTransferFrom() instead of transferFrom() for NFTs; #38

[outdoteth]

high quality report

[HardlyDifficult]

Moved "Recommend using safeTransferFrom() instead of transferFrom() for NFTs" to #318

[HardlyDifficult]

RE "Vaults can be created with non-existent/destructed tokens", unfortunately we're going to keep this as a QA submission. Per the C4 guidance "part of auditing is demonstrating proper theory of how an issue could be exploited" and that does not seem to be explored here as it was in the primary report.