Cally contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L119-L121 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282-L286 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L124-L128

Vulnerability details

Impact

Since there is no cap on feeRate, the owner can set any feeRate he wants by calling setFee(). And feeRate is used in exercise() to collect protocol fees. A malicious owner can monitor the transactions. When he notices a transaction involving a great amount of ETH, he can then launch a front-running attack. Set a 100% fee rate to steal all the ETHs.

Proof of Concept

• Alice is a malicious owner.
• Alice notices Bob calling exercise() with a great amount of ETH. (e.g., 6765 ether)
• Alice launches a front-running attack. Call setFee() to set a 100% fee rate before Bob’s transaction.

There is no cap on feeRate when calling setFee()

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

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

• Then the protocol fee in exercise() take all the ETHs sent from Bob

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

    function exercise(uint256 optionId) external payable {
        …
        // collect protocol fee
        uint256 fee = 0;
        if (feeRate > 0) {
            fee = (msg.value * feeRate) / 1e18;
            protocolUnclaimedFees += fee;
        }
        …
    }

• In the end, Alice can collect those fees.

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

    function withdrawProtocolFees() external onlyOwner returns (uint256 amount) {
        amount = protocolUnclaimedFees;
        protocolUnclaimedFees = 0;
        payable(msg.sender).safeTransferETH(amount);
    }

Tools Used

None

Recommended Mitigation Steps

Add a timelock on setFee() or set a cap on feeRate to remove the benefit of front-running.

Lines of code

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

Vulnerability details

Impact

Users can re-vault vault token in this protocol. However, attackers can deceive buyers to pay high ETH but will not be able to get the underlying asset.

Proof of Concept

1. Alice (Attacker) pack 2 transactions into same block:
   • first transaction: calls createVault to vault a NFT which worth 100 ETH, with parameters:
     • dutchAuctionStartingStrikeIndex is set to 0 (which strikeOptions is 1 ETH)
     • a long durationDays, e.g. 255 days
   • then Alice will get a vaultId 1 token, and Alice do another transaction: call buyOption(1) to get a optionId 2 token
2. Alice re-vault the vaultId 1 token with strike 89 ETH, and get a vaultId 3 token
3. Bob see that the auction of vaultId 3 token is 89 ETH, but the vaultId 3 token can get the NFT which worth 100 ETH, so Bob pays 89 ETH, calls buyOption(3), and exercise(4) to get the vaultId 1 token. Then, Bob calls initiateWithdraw(1) and waits for the optionId 2 token to expire (which durationDays is set to 255 days in step 1).
4. Alice monitors that someone bought the vaultId 1 token, then Alice quickly calls exercise(2). Finally, Alice just pays Bob 1 ETH, and gets the NFT back. Alice also gets 89 ETH which is paid by Bob from the vaultId 3 token.

Tools

vim

Recommended Mitigation Steps

Check the token address should not be address(this) in createVault.

Lines of code

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

Vulnerability details

Impact

If a user sends more ETH than the user has to, the contract just accepts it. The user will lose more ETH accidentally.

Proof of Concept

The buyOption function in Cally.sol:

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

Tools Used

vim

Recommended Mitigation Steps

Use == rather than >=:

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

Summary

We list 1 low-critical finding:

• (low) Users will not be able to call buyOption

(low) Users will not be able to call buyOption

Impact

When users call buyOption, it will be reverted if durationDays is more than 195.

Proof of Concept

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

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

(vault.durationDays * 1 days) will fit in uint24 (max 16777216). It will overflow if vault.durationDays is more than 195 (195 * 86400 = 16848000).

Recommended Mitigation Steps

Convert to uint32:

238         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#L119-L121 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L282-L286

Vulnerability details

Impact

The feeRate is unlimited in Cally.sol, leading to maliciously moving large taxes from this contract.

Proof of Concept

There is no cap on feeRate

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

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

When collecting protocol fee, unlimited feeRate could lead to maliciously obtaining large amounts of ETH from users

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

    function exercise(uint256 optionId) external payable {
        …
        // collect protocol fee
        uint256 fee = 0;
        if (feeRate > 0) {
            fee = (msg.value * feeRate) / 1e18;
            protocolUnclaimedFees += fee;
        }
        …
    }

Tools Used

None

Recommended Mitigation Steps

There should be a reasonable cap on feeRate. (e.g., 10%)

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens have a fee on each transfer. The protocol doesn’t handle the fee when transferring this kind of ERC20 tokens, leading to the inconsistent amount of token actually received in the contract. This will cause users to fail to exercise options due to not enough tokens in the protocol, or the protocol will lose more tokens to compensate users for exercising options.

Proof of Concept

In the createVault function, the amount of token is recorded in tokenIdOrAmount. However, if the ERC20(vault.token) collects a fee on every transfer. The actual amount of token received is not equal to tokenIdOrAmount. vault.tokenIdOrAmount will record more amount of token than it received.

        Vault memory vault = Vault({
            tokenIdOrAmount: tokenIdOrAmount,
            ...
        });

        ...

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

Tools Used

vim

Recommended Mitigation Steps

Use balanceAfter - balanceBefore rather than using amount directly:

function retrieveTokens(address sender, uint256 amount) public {
 uint256 balanceBefore = deflationaryToken.balanceOf(address(this));
 deflationaryToken.transferFrom(sender, address(this), amount);
 uint256 balanceAfter = deflationaryToken.balanceOf(address(this));
 amount = balanceAfter.sub(balanceBefore);
 totalTokenTransferred += amount;
}

Token Transfer Calculation Accuracy in this article: https://blog.chain.link/defi-security-best-practices/

Lines of code

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

Vulnerability details

Impact

It doesn't check whether users will handle ERC721 received. If a user is a contract but doesn’t handle ERC721 received, the ERC721 token will be frozen when receiving ERC721 tokens.

Proof of Concept

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

It doesn’t check whether the receiver has implemented the onERC721Received function. If a user buys an option but the user doesn't handle ERC721 received, the ERC721(vault.token) will be frozen after the user exercises the option.

Tools Used

vim

Recommended Mitigation Steps

Use safeTransferFrom rather than transferFrom when transferring ERC721 tokens to users.

Lines of code

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

Vulnerability details

Impact

When calling createVault(), tokenType could be different from token’s type. If a user accidentally used the wrong tokenType, it could lead to two different results.

If token is an ERC20 token and the user uses TokenType.ERC721 as tokenType. It is less harmful, since ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount) still works when vault.token is actually ERC20 token.

However, if token is an ERC721 token and the user uses TokenType.ERC20 as tokenType. When doing creatVault(), ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount) works fine. But when doing exercise() or withdraw(), ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount); doesn’t work since ERC721 doesn’t implement safeTransfer() function. In consequence, the ERC721 token is frozen in the vault.

Proof of Concept

createVault() does not confirm whether tokenType and token’s type are the same. But the token can still be transferred into this contract. Since transferFrom() is implemented in ERC20 and safeTransferFrom() is implemented in ERC721 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L158-L201

    function createVault(
        uint256 tokenIdOrAmount,
        address token,
        uint8 premiumIndex,
        uint8 durationDays,
        uint8 dutchAuctionStartingStrikeIndex,
        uint256 dutchAuctionReserveStrike,
        TokenType tokenType
    ) external returns (uint256 vaultId) {
        require(premiumIndex < premiumOptions.length, "Invalid premium index");
        require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
        require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
        require(durationDays > 0, "durationDays too small");
        require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");

        Vault memory vault = Vault({
            tokenIdOrAmount: tokenIdOrAmount,
            token: token,
            premiumIndex: premiumIndex,
            durationDays: durationDays,
            dutchAuctionStartingStrikeIndex: dutchAuctionStartingStrikeIndex,
            currentExpiration: uint32(block.timestamp),
            isExercised: false,
            isWithdrawing: false,
            tokenType: tokenType,
            currentStrike: 0,
            dutchAuctionReserveStrike: dutchAuctionReserveStrike
        });

        // vault index should always be odd
        vaultIndex += 2;
        vaultId = vaultIndex;
        _vaults[vaultId] = vault;

        // give msg.sender vault token
        _mint(msg.sender, vaultId);

        emit NewVault(vaultId, msg.sender, token);

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

However when doing exercise() or withdraw(), it always reverts since ERC721 doesn’t implement safeTransfer(). The ERC721 token is frozen in the contract.

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

    function exercise(uint256 optionId) external payable {
        …
        // 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/main/contracts/src/Cally.sol#L345

    function withdraw(uint256 vaultId) external nonReentrant {
        …
        // 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);
    }

Tools Used

None

Recommended Mitigation Steps

Confirm whether tokenType and token’s type are the same in createVault().