transferFrom() is used for ERC721 instead of safeTransferFrom(), which can cause user's NFT to be frozen

[code423n4]

Lines of code

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

Vulnerability details

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

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 memory vault = _vaults[vaultId];

    // 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;

    // 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);
}

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

function withdraw(uint256 vaultId) external nonReentrant {
    // vaultId should always be odd
    require(vaultId % 2 != 0, "Not vault type");

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

    Vault memory vault = _vaults[vaultId];

    // check vault can be withdrawn
    require(vault.isExercised == false, "Vault already exercised");
    require(vault.isWithdrawing, "Vault not in withdrawable state");
    require(block.timestamp > vault.currentExpiration, "Option still active");

    // burn option and vault
    uint256 optionId = vaultId + 1;
    _burn(optionId);
    _burn(vaultId);

    emit Withdrawal(vaultId, msg.sender);

    // claim any ETH still in the account
    harvest();

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

In withdraw() and exercise(), _to is fixed to msg.sender

However, if _to is a contract address does not support ERC721, the NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Recommendation

• Add a recipient parameter;
• Using safeTransferFrom() for ERC721

Consider changing to:

function exercise(uint256 optionId, address recipient) 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 memory vault = _vaults[vaultId];

    // 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;

    // 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).safeTransferFrom(address(this), recipient, vault.tokenIdOrAmount)
        : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
}

function withdraw(uint256 vaultId, address recipient) external nonReentrant {
    // vaultId should always be odd
    require(vaultId % 2 != 0, "Not vault type");

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

    Vault memory vault = _vaults[vaultId];

    // check vault can be withdrawn
    require(vault.isExercised == false, "Vault already exercised");
    require(vault.isWithdrawing, "Vault not in withdrawable state");
    require(block.timestamp > vault.currentExpiration, "Option still active");

    // burn option and vault
    uint256 optionId = vaultId + 1;
    _burn(optionId);
    _burn(vaultId);

    emit Withdrawal(vaultId, msg.sender);

    // claim any ETH still in the account
    harvest();

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

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38