Cally contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function createVault(
    uint256 tokenIdOrAmount,
    address token,
    ...
) external returns (uint256 vaultId) {
    ...
    Vault memory vault = Vault({
        ...
    });

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

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

import "solmate/utils/SafeTransferLib.sol";

...

contract Cally is CallyNft, ReentrancyGuard, Ownable {
    using SafeTransferLib for ERC20;
    ...

When creating a new vault, solmate's SafeTransferLib is used for pulling vault.token from the caller's account, this issue won't exist if OpenZeppelin's SafeERC20 is used instead.

That's because there is a subtle difference between the implementation of solmate's SafeTransferLib and OZ's SafeERC20:

OZ's SafeERC20 checks if the token is a contract or not, solmate's SafeTransferLib does not.

See: https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As a result, when the token's address has no code, the transaction will just succeed with no error.

This attack vector was made well-known by the qBridge hack back in Jan 2022.

For our project, this alone still won't be a problem, a vault created and wrongfully accounted for a certain amount of balance for a non-existing token won't be much of a problem, there will be no fund loss as long as the token stays that way (being non-existing).

However, it's becoming popular for protocols to deploy their token across multiple networks and when they do so, a common practice is to deploy the token contract from the same deployer address and with the same nonce so that the token address can be the same for all the networks.

For example: $1INCH is using the same token address for both Ethereum and BSC; Gelato's $GEL token is using the same token address for Ethereum, Fantom and Polygon.

A sophisticated attacker can exploit it by taking advantage of that and setting traps on multiple potential tokens to steal from the future users that deposits with such tokens.

PoC

Given:

• ProjectA has TokenA on another network;
• ProjectB has TokenB on another network;
• ProjectC has TokenC on another network;

1. The attacker createVault() for TokenA, TokenB, and TokenC with 10000e18 as tokenIdOrAmount each;
2. A few months later, ProjectB lunched TokenB on the local network at the same address;
3. Alice created a vault with 11000e18 TokenB;
4. The attacker called initiateWithdraw() and then withdraw() to receive 10000e18 TokenB.

In summary, one of the traps set by the attacker was activated by the deployment of TokenB and Alice was the victim. As a result, 10000e18 TokenB was stolen by the attacker.

Recommendation

Consider using OZ's SafeERC20 instead.

Lines of code

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

Vulnerability details

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

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

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

    if (feeRate > 0) {
        fee = (msg.value * feeRate) / 1e18;
        protocolUnclaimedFees += fee;
    }

    // increment vault beneficiary's ETH balance
    ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;

When feeRate > 1e18, fee will be larger than msg.value and L289 will revert.

Recommendation

Consider adding an upper-bound to feeRate in setFee():

function setFee(uint256 feeRate_) external onlyOwner {
    require(feeRate_ <= 5 * 1e18 / 100, "invalid feeRate"); // upper bound: 5%
    feeRate = feeRate_;
}

Lines of code

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

Vulnerability details

Cally.sol#createVault() allow users deposit any token to sell.

In the current implementation, createVault() assumes that the received amount is the same as the transfer amount, and uses it as exercise() or withdraw() amounts.

However, there are ERC20 tokens that charge fee for every transfer() or transferFrom().

As a result, later exercise() / withdraw() may revert when trying to transfer token to option owner / vault owner due to insufficient balance.

Furthermore, if there are multiple vaults using the same token, earlier vaults will consume the token balance added by other Vaults. In the edge case, a later vault may lose all the tokens they added as other vaults may consume all their deposits, in essence, losing their funds.

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/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);
    }

PoC

Given:

• XYZ have 10% transfer fee
• Alice and Bob are creating vaults

1. Alice calls createVault() to create vault(vaultId: 3) with 10,000 XYZ:

• _vaults[3].tokenIdOrAmount = 10,000
• Cally.sol contract got 9,000 XYZ in balance

2. Bob calls createVault() to create vault(vaultId: 5) with 10,000 XYZ:

• _vaults[5].tokenIdOrAmount = 10,000
• Cally.sol contract now have 18,000 XYZ in balance

3. When Alice withdraw(3), 10,000 XYZ will be transferred to Alice:

• Cally.sol contract now have 8,000 XYZ in balance.

4. When Bob withdraw(5), transaction will fail due to insufficient balance of XYZ.

Recommendation

Consider comparing before and after balance to get the actual transferred amount, or adding a whitelist for tokens.

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

[N] Misleading error message

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

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");

Recommendation

Change to:

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too large");

[L] Unreachable code

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

function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {
    Vault memory vault = _vaults[vaultId];

    // vaultId should always be odd
    require(vaultId % 2 != 0, "Not vault type");

    // check vault exists
    require(ownerOf(vaultId) != address(0), "Vault does not exist");
    // ...
}

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/lib/solmate/src/tokens/ERC721.sol#L35-L37

    function ownerOf(uint256 id) public view virtual returns (address owner) {
        require((owner = _ownerOf[id]) != address(0), "NOT_MINTED");
    }

When _ownerOf[vaultId] is 0, the tx will revert at L36 in ownerOf() with "NOT_MINTED".

In buyOption(), at L214, the check with the error message: "Vault does not exist" is unreachable code.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S-1] Cally.sol#getPremium() Implementation can be simpler and save some gas

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

    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        Vault memory vault = _vaults[vaultId];
        return premiumOptions[vault.premiumIndex];
    }

• vault is redundant as it's only been used once;
• only premiumIndex is used, no need to SLOAD and copy the whole Vault to memory
• Unneeded named return parameter premium

Recommendation

Change to:

    function getPremium(uint256 vaultId) public view returns (uint256) {
        return premiumOptions[_vaults[vaultId].premiumIndex];
    }

[M-2] Cache storage varibales can save gas

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

uint256[] public premiumOptions = [0.01 ether, 0.025 ether, 0.05 ether, 0.075 ether, 0.1 ether, 0.25 ether, 0.5 ether, 0.75 ether, 1.0 ether, 2.5 ether, 5.0 ether, 7.5 ether, 10 ether, 25 ether, 50 ether, 75 ether, 100 ether];
    // prettier-ignore
uint256[] public strikeOptions = [1 ether, 2 ether, 3 ether, 5 ether, 8 ether, 13 ether, 21 ether, 34 ether, 55 ether, 89 ether, 144 ether, 233 ether, 377 ether, 610 ether, 987 ether, 1597 ether, 2584 ether, 4181 ether, 6765 ether];

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

require(premiumIndex < premiumOptions.length, "Invalid premium index");

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

        require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
        require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");

strikeOptions and premiumOptions can be cached to save gas from unnecessary SLOAD.

[S-3] Cally.sol#getDutchAuctionStrike() Implementation can be simpler and save some gas

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

        uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;
        uint256 progress = (1e18 * delta) / AUCTION_DURATION;
        uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

        // max(auctionStrike, reserveStrike)
        strike = auctionStrike > reserveStrike ? auctionStrike : reserveStrike;

Recommendation

Change to:

        uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;

        if (delta == 0) return reserveStrike;

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

        // max(auctionStrike, reserveStrike)
        strike = auctionStrike > reserveStrike ? auctionStrike : reserveStrike;

When delta == 0, lots of arithmetic operations can be avoided.

[M-4] Setting uint256 variables to 0 is redundant

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

    uint256 public feeRate = 0;
    uint256 public protocolUnclaimedFees = 0;

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

        uint256 fee = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

[M-5] Avoid unnecessary storage reads can save gas

For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

In Cally.sol#createVault(), vaultIndex is read twice.

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

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

Recommendation

Change to:

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

[M-6] Remove unnecessary variable can make the code simpler and save some gas

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

        // check option associated with the vault has expired
        uint32 auctionStartTimestamp = vault.currentExpiration;
        require(block.timestamp >= auctionStartTimestamp, "Auction not started");

auctionStartTimestamp is unnecessary as it's being used only once.

Recommendation

Change to:

        // check option associated with the vault has expired
        require(block.timestamp >= vault.currentExpiration, "Auction not started");