Cally contest - jah's results

Lines of code

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

Vulnerability details

Impact

The function exercise is using transferFrom instead of safeTransferFrom function when transferring ERC721 function and if the exerciser is a smart contract and not aware of incoming ERC721 token, the token could be lost forever

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

use safeTransferFrom instead of transferFrom

Lines of code

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

Vulnerability details

Impact

The function exercise in Cally.sol Logic for handling Fee is wrong instead of taking the fee from the exerciser it is substracting from the beneficiary strike price
ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee; so the exercise will not pay any fee and the beneficiary will not get the strike price amount (As the developer says the exerciser is the one that should pay the fee)

Proof of Concept

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

Tools Used

Manual analysis

Recommended Mitigation Steps

function exercise(uint256 optionId) external payable {
    require(optionId % 2 == 0, "Not option type");
    require(msg.sender == ownerOf(optionId), "You are not the owner");
    uint256 vaultId = optionId - 1;
    Vault memory vault = _vaults[vaultId];
    require(block.timestamp < vault.currentExpiration, "Option has expired");
   _burn(optionId);
    vault.isExercised = true;
    _vaults[vaultId] = vault;
    uint256 fee = 0;
    if (feeRate > 0) {
        fee = (msg.value * feeRate) / 1e18;
        protocolUnclaimedFees += fee;
    }
require(msg.value >= vault.currentStrike +  fee, "Incorrect ETH sent for strike");
    ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;
    emit ExercisedOption(optionId, msg.sender);
    vault.tokenType == TokenType.ERC721
        ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
        : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
}