Cally contest - VAD37's results

Lines of code

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

Vulnerability details

createVault does not require check to prevent user from setting Cally.sol as token address. It is bad idea all around.

Impact

User can create vault using cally vault NFT or optionId NFT as auction reward. This open up many creative ways to exploit contract.

I did not found ways to exploit buyOption, exercise and withdraw to steal optionId from burn address, or worst double withdrawal. But it doesn't mean someone else can figure it out.

The worst I have found is I can spam vault creation with just one NFT. The Cally.sol will hold lots of its own vault NFT.

Unexpected use of contract = Medium severity.

Recommended Mitigation Steps

Block user from create vault with cally address. It is easy to do so.

require(token != address(this), "Invalid token");

Proof of Concept

Deployed Rinkeby testnet contract

Spam test

Judge has assessed an item in Issue #182 as Medium risk. The relevant finding follows:

L-Can send ETH more than buyOption premium required

Link. require(msg.value >= premium) should be require(msg.value == premium) to prevent user send too much eth. Only beneficiary benefit from this. Contract should protect user from these nuance mistakes.

Lines of code

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

Vulnerability details

vault.durationDays is uint8. When multiply by 1 days, it overflows when result value > type(uint24).max = 16777216 or 195 * 86400 = 16848000.

Impact

No one can buyOption for vault with duration >= 195 days

Tools used

Forge fuzz test caught by accident

Proof

Add this code to CreateVault.t.sol. Run with forge test --match-test Fuzz -vvv

    function testItFuzzCreatesVault(
        uint8 premiumIndex,
        uint8 durationDays,
        uint8 dutchAuctionStartingStrikeIndex
    ) public {
        vm.assume(premiumIndex < 17);
        vm.assume(durationDays > 190);
        vm.assume(dutchAuctionStartingStrikeIndex < 19);
        uint256 vaultId = c.createVault(
            1,
            address(bayc),
            premiumIndex,
            durationDays,
            dutchAuctionStartingStrikeIndex,
            0,
            Cally.TokenType.ERC721
        );

        // assert
        Cally.Vault memory vault = c.vaults(vaultId);
        emit log_named_uint("days",durationDays);
        uint premium = c.getPremium(vaultId);
        // Other test does not run a full system. So add buyOption here show the problem when duration days > 195
        uint256 optionId = c.buyOption{value: premium}(vaultId);
        assertEq(optionId, vaultId + 1, "Option ID should be 1 less than vault ID");
    }

Recommended Mitigation Steps

Explicit convert uint8 to uint32 before multiply solve the problem.

(vault.durationDays * 1 days) with ( uint32(vault.durationDays) * 1 days)

The reason why it overflow at uint24 is unknown to me and it shouldn't happen.

To be safe, the project should add some system testing. With current unit testing, there might be some weird bugs. Hence, I only caught it by chance while testing something else.

Lines of code

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

Vulnerability details

Lots of meme token put fee on transfer. The receiving amount always lesser than what user input.

Impact

If any ERC20 token can show up on the website, user can buyOption of token that cannot be withdrawed

POC

User create vault with 1000 token. Cally mint vault as 1000 token But contract receive 900 token. Lost 10% fee on transfer.

Any user can still buyOption on vault as normal. Vault beneficiary got premium fee.

When exercise, vault does not have enough token to transfer to buyer.

Either beneficiary or buyer have to add extra 11% original token to cover transfer fee. Otherwise, no one can withdraw this vault.

Recommended migitation steps

Either project explicit define whitelist token on website or support all token include meme coin. (Some mainstream token like USDT also have fee on transfer but currently at 0%)

I would recommend just flat out warning user from bidding on meme coin and ignore this. This would consider as user fault and not contract fault.

Otherwise, change these line to a simple version of this.

if(vault.tokenType == TokenType.ERC721 )
{
    ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
}
else {
    // This also prevent EOA set as token
    uint before = ERC20(vault.token).balanceOf(address(this));            
    ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
    uint afterTransfer = ERC20(vault.token).balanceOf(address(this));
    uint diff = afterTransfer - before;
    // Note this is not Vault memory location but Vault storage pointer location
    // No need to set _vaults[vaultId] = vault; after transfer 
    vault.tokenIdOrAmount = diff; 
}

QA

1. QA
   1. L-Missing SetMaxFee
   2. L-Can send ETH more than buyOption premium required
   3. N-Consider use ERC721 SafeTransferFrom instead of transferFrom
   4. L-Can create vault with EOA address as ERC20 token address

L-Missing SetMaxFee

If setFee set higher than 1e18. exercise will underflow.

During live production, it is possible to accidentally set fee higher than it suppose to be.

Cap max fee 5%,10%,20%(20e17) would prevent any accident or malicious owner handling.

L-Can send ETH more than buyOption premium required

Link. require(msg.value >= premium) should be require(msg.value == premium) to prevent user send too much eth. Only beneficiary benefit from this. Contract should protect user from these nuance mistakes.

N-Consider use ERC721 SafeTransferFrom instead of transferFrom

withdraw and exercise already implement check and effect parttern. There is no risk of reentrancy with ERC721 safeTransferFrom. This provides service for user who use special NFT wallet. Or simply prevent user from withdraw NFT to unsupported contract.

L-Can create vault with EOA address as ERC20 token address

CreateVault does not check if token have extCodeSize > 0 or not. User can create vault with EOA address as ERC20 token address (tokenType = ERC721 throw error). Interact vault with buyOption and exercise work as normal. This happens due to external raw call to EOA always return success. Interface ERC721 wrap address call does not.

Contract should prevent create accident vault with non-contract address.

Gas

1. Gas
   1. solc 0.8.13 require comparision !=0 better than >0 for 1 gas
   2. inline auctionStartTimeStamp
   3. replace memory with storage pointer
      1. buyOptions (-3718 gas)
      2. withdraw (-4242 gas)
      3. exercise (-3505 gas)
   4. createVault remove memory copy (-47 gas)

solc 0.8.13 require comparision !=0 better than >0 for 1 gas

I am really not sure why everyone keep posting this tweet as proof.

But I test it and !=0 only save 1 gas with max optimization in version 0.8.13. Only work with require in createVault and not if comparision.

inline auctionStartTimeStamp

Inline these line because cache uint32 auctionStartTimestamp only use once with require check.

require(block.timestamp >= vault.currentExpiration, "Auction not started");

replace memory with storage pointer

Use storage memory pointer instead of memory copy struct save quite lots of gas.

buyOptions (-3718 gas)

function buyOption(uint256 vaultId) external payable returns (uint256 optionId) { 
        Vault storage vault = _vaults[vaultId]; //@gas use storage instead of memory 

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

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

        // check that the vault still has the NFTs as collateral
        require(vault.isExercised == false, "Vault already exercised");

        // check that the vault is not in withdrawing state
        require(vault.isWithdrawing == false, "Vault is being withdrawn");

        // check enough eth was sent to cover premium
        uint256 premium = _getPremium(vault); //@gas boilerplate _internal func version with storage pointer could save gas
        // premiumOptions[vault.premiumIndex]
        require(msg.value >= premium, "Incorrect ETH amount sent"); //@audit-ok premium should be equal unnecessary send more than required

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

        // set new currentStrike
        vault.currentStrike = getDutchAuctionStrike(
            strikeOptions[vault.dutchAuctionStartingStrikeIndex],
            vault.currentExpiration + AUCTION_DURATION, //@gas inline startTimetamp
            vault.dutchAuctionReserveStrike
        );

        // set new expiration
        vault.currentExpiration = uint32(block.timestamp) + (vault.durationDays * 1 days);

        // update the vault with the new option expiration and strike
        // _vaults[vaultId] = vault; //@gas replace with storage pointer

        // force transfer the vault's associated option from old owner to new owner
        // option id for a respective vault is always vaultId + 1
        optionId = vaultId + 1;
        _forceTransfer(msg.sender, optionId);

        // increment vault beneficiary's unclaimed premiums
        address beneficiary = getVaultBeneficiary(vaultId);
        ethBalance[beneficiary] += msg.value;

        emit BoughtOption(optionId, msg.sender, vault.token);
    }
    
    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        Vault memory vault = _vaults[vaultId]; // @gas can calldata or storage pointer cheaper than memory copy?
        return premiumOptions[vault.premiumIndex];
    }

    function _getPremium(Vault storage vault) internal view returns (uint256 premium) {        
        return premiumOptions[vault.premiumIndex];
    }

withdraw (-4242 gas)

Only have to replace Vault memory with Vault storage here

exercise (-3505 gas)

• Replace exercise Vault memory vault = _vaults[vaultId]; with Vault storage vault = _vaults[vaultId];
• Remove store memory Vault line

createVault remove memory copy (-47 gas)

This does not save much gas and entirely style optional. Only report for completion.

• Remove memory vault cache.
• Prefer local variable than struct

    function createVault(
        uint256 tokenIdOrAmount,
        address token, //@audit-ok does not check zero-address token 
        uint8 premiumIndex,
        uint8 durationDays, //@audit-ok durationDays can be any.
        uint8 dutchAuctionStartingStrikeIndex,
        uint256 dutchAuctionReserveStrike, //@audit-ok what happen when reserve strike is 0?
        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"); //@audit-ok dutchAuctionReserveStrike cannot be max? could be <=?
        require(durationDays != 0, "durationDays too small"); //@gas use !0 instead of > 0
        require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");//@gas can we use < number for enum require?
        
        // vault index should always be odd
        vaultIndex += 2;
        vaultId = vaultIndex;
        // @gas inline set Vault 
        _vaults[vaultId] = 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
        });

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

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

        // transfer the NFTs or ERC20s to the contract
        //@gas use local variable instead of read from vault struct
        tokenType == TokenType.ERC721
            ? ERC721(token).transferFrom(msg.sender, address(this), tokenIdOrAmount) //@audit vault.token can this address be cally address?
            : ERC20(token).safeTransferFrom(msg.sender, address(this), tokenIdOrAmount);// @audit we can transfer our own NFTvault to vault. And do it again in a loop. Hidden vault NFT behind several layer
    }