Cally contest - BondiPestControl's results

Lines of code

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

Vulnerability details

Impact

There are no checks to ensure the the vault.token contract exists when creating a vault.

Token address are deterministic in the EVM and can be known ahead of time. As a result it is possible for a user to call createVault() with token set to a ERC20 contract that has not yet been deployed but will be deployed in the future.

Now if a user calls createVault() before a new ERC20 token has been created it will credit the vault with these tokens. The cause is from ERC20(token).safeTransferFrom() which will succeed if token has zero bytecode at the address.

The later when the ERC20 token is deployed and another user calls createVault() with this token, the attacker is able to call withdraw() and there will be no tokens left in the contract for the genuine vault.

Proof of Concept

There are no checks before calling safeTransferFrom to ensure vault.token has bytecode.

             ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);

Recommended Mitigation Steps

Use the extcodesize EVM opcode to ensure that there is bytecode at the vault.token address during createVault() otherwise revert the transaction.

Lines of code

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

Vulnerability details

Impact

It is possible for the owner of the contract to set the feeRate equal to or above 100%.

The impact of setting the contract above 100% (1e18) is that the fee which is msg.value * feeRate / 1e18 may be greater than the msg.value. There will be an overflow in line #289 causing all calls to exercise() to fail.

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

Furthermore, the owner may perform a rug pull attack by setting the feeRate = 1e18 (exactly 100%). This would allow the owner to exercise any options without paying any funds. That is due to the fact that the entire strike amount will be considered fees and attributed to protocolUnclaimedFees which the owner may later withdraw. The owner would then also receive the token which was held in the vault.

Proof of Concept

There are no restrictions on the function setFee().

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

The owner may then claim all fees and receive tokens in exercise(), the only cost to the owner would by the premium amount when they purchase the option from the auction.

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

Recommended Mitigation Steps

Consider setting an reasonable upper limit of the feeRate such as 5% in setFee().

Furthermore, consider have vaults lock in the feeRate when they first createVault(). This would prevent the owner manipulating the value when a user is trying to exercise the option.

Lines of code

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

Vulnerability details

Impact

It is possible for a user purchasing an option to accidentally overpay the premium during buyOption().

Any excess funds paid for in excess of the premium will be transferred to the vault creator.

The premium is fixed at the time the vault is first created by vault.premiumIndex. Hence there is no need to allow users to overpay since there will be no benefit.

Proof of Concept

buyOption() allows msg.value > premium

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

Recommended Mitigation Steps

Consider modifying the check such that the msg.value is exactly equal to the premuim. e.g.

        uint256 premium = getPremium(vaultId);
        require(msg.value == premium, "Incorrect ETH amount sent");

Lines of code

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

Vulnerability details

Impact

There is a potential overflow in the function buyOption() which will revert if triggered.

The overflow occurs when durationDays >= 195. As a result it is impossible for any user to call buyOption() if the duration exceeds this value.

The vault creator will still be able to withdraw through withdraw()

Proof of Concept

The following line may cause an overflow due to (vault.durationDays * 1 days).

        vault.currentExpiration = uint32(block.timestamp) + (vault.durationDays * 1 days);

The issue occurs since vault.durationDays is of type uint8 and 1 days is of type uint24 (solidity defaults to the lowest size that the value fits).

As a result vault.durationDays * 1 days are treated as uint24 * uint24.

1 days = 86,400

and

195 * 86,400 = 16,848,000

Since,

2**24 = 16,777,216

there is an overflow for the uint24. Overflows in solidity 0.8.13 will cause the transaction to revert.

Since vault.durationDays cannot be changes all calls to buyOption() for this token ID will fail if durationDays >= 195

Recommended Mitigation Steps

Consider updating the multiplication such that durationDays is first cast to uint32. This will prevent an overflow since the max value of durationDays = 255 multiplied by 1 days is less than 2**32 and so will not overflow a uint32.

e.g.

vault.currentExpiration = uint32(block.timestamp) + (uint32(vault.durationDays) * 1 days);

Lines of code

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

Vulnerability details

Impact

When a user creates a ERC20 vault by calling createVault(), tokens are transferred from the caller to the contract equivalent tokenIdOrAmount. If the token contract used charges a fee for the transfer, the transfer initiated by an exercised option will ultimately fail.

As a result, users could set up a honey pot where they create a vault using a fee-on-transfer token, sell options on this vault with whatever strike price and premium and with certainty keep their tokens and the premium amount as an exercise of this option will fail. The vault owner will always profit from this trade and the option buyer will always lose the premium they paid.

Because this leads to a loss of funds by an honest option buyer, I believe high severity is deserving.

Recommended Mitigation Steps

Consider snapshotting the before and after balance for all ERC20 transfers in and out of the contract. This will ensure the balance for each vault is strictly correct, regardless if a vault is created upon a fee-on-transfer token.

Lines of code

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

Vulnerability details

Impact

While the Cally.sol contract is compatible with standard ERC20 and ERC721 tokens, it is not able to handle popular non-standard ERC721 tokens such as cryptopunks. The typical transferFrom() call will fail as a result, and these users will likely opt to use another protocol which does support options on their specific NFT.

Recommended Mitigation Steps

NFTX protocol has implemented a way to handle the transfer of both standard and non-standard ERC721 tokens. The relevant implementation can be found here. The solution provided also utilises OpenZeppelin's safeTransferFrom() function on most transfers as this ensures the function reverts on failure.