Cally contest - hake's results

Lines of code

Cally.sol#L127

Vulnerability details

Impact

Attacker could set the feeRate to 100% without any time delay, front run any premiums/strikes being processed and steal premiums/strike from vault owner.

Proof of Concept

Cally.sol#L119-L128

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

    /// @notice Withdraws the protocol fees and sends to current owner
    function withdrawProtocolFees() external onlyOwner returns (uint256 amount) {
        amount = protocolUnclaimedFees;
        protocolUnclaimedFees = 0;
        payable(msg.sender).safeTransferETH(amount);
    }

No time delay for feeRate or withdrawProtocolFees. withdrawProtocolFees sends ETH to msg.sender.

Recommended Mitigation Steps

Consider implementing a multisig system. Consider introducing a time delay in setFee. Consider setting withdrawProtocolFees recipient address to a different fixed address instead of msg.sender. Add maxFee parameter to accommodate vault owners tolerance.

Lines of code

Cally.sol#L224-L226

Vulnerability details

Impact

If option buyer sends an amount bigger than premium the difference is not refunded to buyer.

This could lead to a big loss for the buyer who could have made a mistake like for example typing an extra zero.

Proof of Concept

require(msg.value >= premium, "Incorrect ETH amount sent");

Recommended Mitigation Steps

Consider refunding the difference to the buyer (more gas expensive) or implementing a check with strict equality: msg.value == premium (less gas expensive).

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

Protocol does not support fee-on-transfer tokens The tokenIdOrAmount established in createVault prevents buyers from exercise their option because address(this) holds less than tokenIdOrAmount due to the transfer fee.

That is also valid for withdraw.

I recommend making it explicitly to users that such tokens are not supported or preferably only allowing a set of whitelisted ERC20s.

Lines of code

Cally.sol#L158-L17

Vulnerability details

Impact

Scenario 1:

• User creates vault for an ERC721, but mistakenly inputs an approved ERC20 address.
• Users ERC20 balance value is equal or bigger than the intended tokenId value.
• User mistakenly sends ERC20 to the Vault
• People buying options from vault creator might take advantage of this by being able to purchase an ERC20 for a lower price than what it is worth depending on inputs such as dutchAuctionReserveStrike and dutchAuctionStartingStrikeIndex.

Scenario 2 (extremely unlikely):

• User creates vault for an ERC20, but mistakenly inputs an ERC721 address.
• Users ERC20 balance value is equal to ERC721 tokenId value.
• People buying options from vault creator might take advantage of this by being able to purchase a ERC721 for a lower price than what it is worth depending on inputs such as dutchAuctionReserveStrike and dutchAuctionStartingStrikeIndex.

Proof of Concept

Cally.sol#L198-L200

vault.tokenType == TokenType.ERC721
  ? ERC721(vault.token).transferFrom(
      msg.sender,
      address(this),
      vault.tokenIdOrAmount
    )
  : ERC20(vault.token).safeTransferFrom(
      msg.sender,
      address(this),
      vault.tokenIdOrAmount
    );

ERC721.transferFrom may actually call a ERC20 transferFrom and vice versa.

Recommended Mitigation Steps

Dont allow user input to be the sole validator on ERC being an ERC20 or ERC721.

If TokenType.ERC20 implement extra inputs and require functions to ensure name, symbol and decimals match inputs. Same thing for TokenType.ERC721 excluding decimals and confirming inputs match name, symbol and tokenId/id.

Only allow whitelisted tokens to be used as collateral.

LOW

L-01: Protocol does not support fee-on-transfer tokens

The tokenIdOrAmount established in createVault prevents buyers from exercise their option because address(this) holds less than tokenIdOrAmount due to the transfer fee.

That is also valid for withdraw.

I recommend making it explicitly to users that such tokens are not supported or preferably only allowing a set of whitelisted ERC20s.

L-02: Vault Tokens can be used as collateral

Cally.sol#L158

Allowing Vault Tokens to be used as collateral may lead to users accidentally giving up his NFT or its strike price by mistake.

Please implement a check in createVault as prevention:

require(token != address(this));

L-03: Use of transferFrom instead of safeTransferFrom

Cally.sol#L295

When buyers exercise the transferFrom function is used instead of safeTransferFrom. In the very unlikely event the receiving contract is somehow not aware of incoming ERC721, the token could be locked.

Consider exchanging transferFrom for safeTransferFrom

L-04: Divide before multiple may lead to loss of precision

https://github.com/code-423n4/2021-11-streaming-findings/issues/28 ----REFERENCE

Cally.sol#L418-L420

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

Performing multiplication before division is generally better to avoid loss of precision because Solidity integer division might truncate.

This truncation might affect auctionStrike.

NON-CRITICAL

N-01: durationDays input is not correctly enforced

Cally.sol#L170

require(durationDays > 0, "durationDays too small");

durationDays input check leads to believe there is a minimum of at least 1 day. However, if user inputs any amount above zero the check will pass.

I recommend hardcoding in a constant the equivalent of 1 day in seconds and using that as a comparison instead of zero.

N-02: Minor Typo

/*************************
    OVVERIDES FUNCTIONS
**************************/

Please change to OVERRIDES.

GAS

Unnecessary caching + inconsistency

Cally.sol#L249-L251 buyOption:

address beneficiary = getVaultBeneficiary(vaultId);
ethBalance[beneficiary] += msg.value;

In the code snippet above the beneficiary is unnecessarily cached because it is only called once.

In the code snippet below we can see the same functionality without the caching.

Cally.sol#L289-L290 exercise:

ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

I suggest using the same implementation from exercise in buyOption.