Cally contest - 0xsanson's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L197-L200

Vulnerability details

A vault can be created for different TokenTypes, namely ERC20 and ERC721. These tokens have different logic, so a user needs to pass the type of the underlying token when creating a vault. At the end of createVault the user's tokens are pulled into the contract (L197):

// transfer the NFTs or ERC20s to the contract
vault.tokenType == TokenType.ERC721
    ? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
    : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);

The issue is that an user can choose to mismatch vault.tokenType and the true vault.token's type.

More importantly, if they use tokenType=ERC721 for a ERC20 that doesn't revert on failure (but returns false for example), they can basically create a vault for free.

Proof of concept

This situation causes two attack vectors:

(1) Alice creates a vault as explained before with a premium and a very favourable strike for the buyer. Bob is monitoring vaults' creations by checking the pair (vault.token, vault.tokenIdOrAmount); he immediately buyOption thinking he made a good deal, but during exercise he doesn't get any token. Alice gained premium + strike for free. (2) Let's assume there are already some ERC20 tokens that don't revert on transferFrom in the contract. Alice can create a vault with vault.tokenIdOrAmount equal the balance of the contract; she then can call initiateWithdraw+withdraw, and the ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount) will actually succeed. Basically she drained the contract of all these tokens.

Recommendations

A possible fix may consist in calling vault.token querying for EIP165's supportsInterface(bytes4 interfaceId) with interfaceId = 0x80ac58cd (ERC721 interfaceId). If the staticall fails or returns false then the token is ERC20, otherwise ERC721.

Another fix can be separating Cally into two contracts, one for ERC20 and the other for ERC721.

Judge has assessed an item in Issue #307 as Medium risk. The relevant finding follows:

(Low) feeRate can be modified for existing vaults

feeRate is a parameter that controls the fee applied on exercise. It can be set by the function:

function setFee(uint256 feeRate_) external onlyOwner {
    feeRate = feeRate_; 
} 

So it can be changed by the owner at any time, changing also the fee payed by existing vaults.

Proof of concept

Alice is a trader looking for a delta neutral position on her NFTs. She opens a vault with strike 10 ETH and fee 0.5%. She's happy getting 9.95 ETH if the option is exercised. During the call lifetime the fee gets increased up to 1%. Now Alice will get 9.90 ETH, an amount which may bring her EV negative.

Recommendations

It's suggested to save feeRate into a vault struct during createVault or buyOption. This value can then be used during exercise instead of the global variable.

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L200

Vulnerability details

Some ERC20 tokens may have fee-on-transfer or change balance without owner intervention. If these tokens are used as underlying in the protocol they can be lost.

Proof of concept

Alice creates a vault with a token that has a 1% fee on transfer. She sends vault.tokenIdOrAmount = 100 during createVault, meaning that the contract balance becomes 99. When the vault is withdrawn (or someone buys and tries to exercise) the transaction fails because ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount) (L345) tries to transfer back 100 tokens. As a consequence these tokens are irretrievable.

Recommendations

Consider if the protocol is interested in supporting these token. If not, it should be well documented to the user to not deposit them. If instead you want to support them, check token.balanceOf(address(this)) before and after a "pull" to compute the true tokenIdOrAmount.

(Low) feeRate can be modified for existing vaults

feeRate is a parameter that controls the fee applied on exercise. It can be set by the function:

function setFee(uint256 feeRate_) external onlyOwner {
    feeRate = feeRate_; 
} 

So it can be changed by the owner at any time, changing also the fee payed by existing vaults.

Proof of concept

Alice is a trader looking for a delta neutral position on her NFTs. She opens a vault with strike 10 ETH and fee 0.5%. She's happy getting 9.95 ETH if the option is exercised. During the call lifetime the fee gets increased up to 1%. Now Alice will get 9.90 ETH, an amount which may bring her EV negative.

Recommendations

It's suggested to save feeRate into a vault struct during createVault or buyOption. This value can then be used during exercise instead of the global variable.

(Low) Mistake in tokenURI encoding

In the tokenURI string calculation, we need to pass the vault premium. At line L464 there's a getPremium(vault.premiumIndex) but the function actually wants a vaultId:

    /// @notice Get the fixed option premium for a vault
    /// @param vaultId The tokenId of the vault to fetch the premium for
    /// @return premium The premium for the vault
    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        Vault memory vault = _vaults[vaultId];
        return premiumOptions[vault.premiumIndex];
    }

As a consequence the premium gets mispriced on UI, leading to unpredictable errors.

Recommendations

Change L464 to premiumOptions[vault.premiumIndex].

(Info) Wrong math terminology: strike curve is not "exponential"

Documentation (L400) claims that during dutch auction, strike decreases in time "exponentially". This term is correct colloquially, but it's technically misused since the curve is actually a parabola instead of an exponential.

Suggested removing the adverb or finding a more appropriate one.

(Info) Missing event when changing feeRate

The function setFee doesn't emit an event when changing feeRate. It's generally suggested to add events for every important parameter that can get changed, for easier off-chain monitoring.

Usage of storage instead of memory can save gas

When calling Vault memory vault = _vaults[vaultId]; all the struct is read from storage to memory. If we are interested in only some values we can use Vault storage vault = _vaults[vaultId]; which saves a reference to storage, meaning that SLOADs happen later only for the value we access.

These are all instances where it can save gas (see @audit comments for diff)

getPremium

    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        Vault storage vault = _vaults[vaultId];  //@audit from memory to storage
        return premiumOptions[vault.premiumIndex];
    }

buyOption

    function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {
        Vault storage vault = _vaults[vaultId];  //@audit from memory to storage

        // vaultId should always be odd
        require(vaultId % 2 != 0, "Not vault type");

        // check vault exists
        require(ownerOf(vaultId) != address(0), "Vault does not exist");

        // check that the vault still has the NFTs as collateral
        require(vault.isExercised == false, "Vault already exercised");

        // check that the vault is not in withdrawing state
        require(vault.isWithdrawing == false, "Vault is being withdrawn");

        // check enough eth was sent to cover premium
        uint256 premium = getPremium(vaultId);
        require(msg.value >= premium, "Incorrect ETH amount sent");

        // check option associated with the vault has expired
        uint32 auctionStartTimestamp = vault.currentExpiration;
        require(block.timestamp >= auctionStartTimestamp, "Auction not started");

        // set new currentStrike
        //@audit also updates storage
        vault.currentStrike = getDutchAuctionStrike(
            strikeOptions[vault.dutchAuctionStartingStrikeIndex],
            vault.currentExpiration + AUCTION_DURATION,
            vault.dutchAuctionReserveStrike
        );

        // set new expiration
        //@audit also updates storage
        vault.currentExpiration = uint32(block.timestamp) + (vault.durationDays * 1 days);

        //@audit no needed - already updated
        // update the vault with the new option expiration and strike
        //_vaults[vaultId] = vault;

        // force transfer the vault's associated option from old owner to new owner
        // option id for a respective vault is always vaultId + 1
        optionId = vaultId + 1;
        _forceTransfer(msg.sender, optionId);

        // increment vault beneficiary's unclaimed premiums
        address beneficiary = getVaultBeneficiary(vaultId);
        ethBalance[beneficiary] += msg.value;

        emit BoughtOption(optionId, msg.sender, vault.token);
    }

exercise

    function exercise(uint256 optionId) external payable {
        // optionId should always be even
        require(optionId % 2 == 0, "Not option type");

        // check owner
        require(msg.sender == ownerOf(optionId), "You are not the owner");

        uint256 vaultId = optionId - 1;
        Vault storage vault = _vaults[vaultId];  //@audit from memory to storage

        // check option hasn't expired
        require(block.timestamp < vault.currentExpiration, "Option has expired");

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

        // burn the option token
        _burn(optionId);

        // mark the vault as exercised
        vault.isExercised = true;
        //_vaults[vaultId] = vault;  //@audit no needed - already updated

        // collect protocol fee
        uint256 fee = 0;
        if (feeRate > 0) {
            fee = (msg.value * feeRate) / 1e18;
            protocolUnclaimedFees += fee;
        }

        // increment vault beneficiary's ETH balance
        ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

        emit ExercisedOption(optionId, msg.sender);

        // transfer the NFTs or ERC20s to the exerciser
        vault.tokenType == TokenType.ERC721
            ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
            : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
    }

withdraw

L325

Vault storage vault = _vaults[vaultId];  //@audit from memory to storage