Cally contest - rfa's results

Lines of code

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

Vulnerability details

Impact

It's possible to input ERC20 token address while set tokenType == TokenType.ERC721.

Proof of Concept

Alice input: Address token = 0xExampleERC20address TokenType tokenType = TokenType.ERC721

The function won't throw any error or message to inform that the tokenType inputed is wrong tokenType. And it also will execute ERC20(vault.token).transferFrom() function although it was assigned as ERC721 token. It's possible because the address is a ERC20 token address (in this case), and the transferFrom() both in ERC20 & ERC721 has the same ABI. Therefore it also possible in some case, user inputing their tokenId (ERC721 third argument in safeTransfer()) as an amount (ERC20)

Tools Used

Manual review

##LOW

Title: vaultId 1 can't be created

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

Currently, vaultId is started by 3. By replacing L188 and 189 can make it start to create the vaultId by number 1 as vaultIndex initialized value.

        vaultId = vaultIndex; //@audit-info: Set vaultId first, then update vaultIndex
	vaultIndex += 2;

##GAS

Title: Var set with default value

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

By just declare var without set the value (if value is default) is way efficient

    uint256 public feeRate;
    uint256 public protocolUnclaimedFees;

Title: Using uint256 for uint inside struct

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

Using uint256 is way more efficient than uint8 or uint 32 inside the struct. The reason for this is that the EVM reads 32 bytes at a time and will have to do some operations to make the data it is reading go down to 8 bits (1 byte).

Source: https://dev.to/javier123454321/solidity-gas-optimizations-pt-3-packing-structs-23f4

RECOMMENDED MITIGATION STEP

struct Vault {
        uint256 tokenIdOrAmount;
        uint256 premiumIndex; // indexes into `premiumOptions`
        uint256 durationDays; // days
        uint256 dutchAuctionStartingStrikeIndex; // indexes into `strikeOptions`
        uint256 currentExpiration;
        address token; //@audit-info: Move address here to save 1 slot
        bool isExercised;
        bool isWithdrawing;
        TokenType tokenType;
        uint256 currentStrike;
        uint256 dutchAuctionReserveStrike;
    }

Title: Using ! operator to check value is false

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

Using ! operator is way more effective than using == to validate bool var value is false

RECOMMENDED MITIGATION STEP

require(!vault.isExercised, "Vault already exercised");

It can save 15 execution gas cost

Title: Using storage pointer to store struct var

Occurrences: https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L208 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L266 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L395 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L325

Reading directly from storage than storing it to memory can save a lot of gas fee (especially if there is a lot data were stored in the struct) RECOMMENDED MITIGATION STEP

        Vault storage vault = _vaults[vaultId]; // @audit-info: Change memory to storage

Title: Using ** operator

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

Instead doing var * var (which is doing 2 times MLOAD) using var**2 is way more effective and increasing readability of the code Change to:

	//@audit-info: has no effect for gas opt by using it on 1e18, but still recommending doing so
	uint256 auctionStrike = (progress**2 * startingStrike) / (1e18**2);

Title: Check the vaultId is odd earlier

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

Before initializing vault (currently at L208) which cost a lot of gas fee. Check it == odd number is way more effective (it doesn't need to read the data from _vaults[vaultId]) Change to:

    function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {
	// vaultId should always be odd
        require(vaultId % 2 != 0, "Not vault type");        

	Vault memory vault = _vaults[vaultId]; //@audit-info: Replace the location of L208 & L211

Title: Using != operator

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

Using != operator is way more effective than > to validate that the var's value is not zero

Title: Using value to validate fixed array.length

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

Instead of SLOADing from storage, put the array.length directly if the array.length is fixed is cost less gas Change to:

        require(premiumIndex < 17, "Invalid premium index");
        require(dutchAuctionStartingStrikeIndex < 19, "Invalid strike index");

The error string has made the code is still clear and readable

Title: Doing MLOAD auctionStartTimestamp instead SLOAD

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

vault.currentExpiration was MSTOREd at L227. Using it's memory var for getDutchAuctionStrike() argument can save gas by avoiding doing SLOAD Change to:

        vault.currentStrike = getDutchAuctionStrike(
            strikeOptions[vault.dutchAuctionStartingStrikeIndex],
            auctionStartTimestamp + AUCTION_DURATION, //@audit-info: change made at this line
            vault.dutchAuctionReserveStrike
        );

Title: Efficiency in getDutchAuctionStrike()

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

If auctionEndTimestamp > block.timestamp is false, the return value will always 0. It's gas efficient to just skip all the math operation if return value is expected 0.

Change to:

    function getDutchAuctionStrike(
        uint256 startingStrike,
        uint32 auctionEndTimestamp,
        uint256 reserveStrike
    ) public view returns (uint256 strike) {
        /*
            delta = max(auctionEnd - currentTimestamp, 0)
            progress = delta / auctionDuration
            auctionStrike = progress^2 * startingStrike
            strike = max(auctionStrike, reserveStrike)
        */
	if(auctionEndTimestamp > block.timestamp){ //@audit-info: Condition checked here 
        	uint256 delta = auctionEndTimestamp - block.timestamp; //@audit-info: Changes made here
        	uint256 progress = (1e18 * delta) / AUCTION_DURATION;
        	uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

        	// max(auctionStrike, reserveStrike)
        	strike = auctionStrike > reserveStrike ? auctionStrike : reserveStrike;
	}
	//@audit-info: strike will has default (0) value if it's not set
    }

Title: Using _ownerOf instead ownerOf()

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

ownerOf() is a function which reverting the transaction if vaultId isn't exist. By using _ownerOf[vaultId] inside the require() or calling ownerOf(vaultId) without require can save gas

Change to:

        require(_ownerOf[vaultId] != address(0), "Vault does not exist");

Or:

	ownerOf(vaultId);