Cally contest - Funen's results

Low

1. Users can create vaults with a malicious token addresses

In Cally.sol a user can call the createVault() function passing in a malicious attacker controlled token as the address token. This will then create a vault setting the evil token address as in storage. Attacker controlled assets should never be allowed in a protocol as they can return arbitrary values when called upon performing other malicious tasks then whats expected.

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

##Tool Used Manual Review

##Recommended Mitigation Consider adding a mapping of approved tokens that can be passed in as an token address to the createVault() function to avoid malicious tokens.

2. Used safeMint() rather than mint()

This POC It can be consider for dev if may necessary for better way

The `_safeMint` can be using if minting causes the recipient of the tokens, if it is a smart contract, to react upon receipt of the tokens.

Here is a general consideration to help decide which one to use:

If YOU are paying for the minting of tokens, use `_mint`. The `_safeMint` might cost you an arbitrary amount of money because of choices made by the recipient of the tokens.

If THEY are paying for the minting of tokens and you expect buyers to be composing functionality with smart contracts, use _safeMint. There is some marginal benefit of allowing the extra features with smart contracts this allows.

If THEY are paying and you expect INDIVIDUAL PEOPLE to buy tokens, then use _mint. The extra features in _safeMint are not expected.

The extra cost from using _safeMint is non-zero. And cost is also always a concern.

since Cally.sol was used to be main actor, it can be used safemint() instead of mint()

##Tool Used Manual Review

3. Title : comment @return was not set

This was not set information for @return, since other function() was set if @return was used so it can be added for good information to the others.

##Tool Used Manual Review

Non Critical

4. TItle : Typo Comment

It should be an overrides function but instead of overrides, dev was used ovverides. It can be remain the same, or it can be changed instead.

##Tool Used Manual Review

1. Title : Reorder struct can be cost less gan

This can be implemented for save gas cost

##Tool Used vsc, gas test

##Recommended Mitigation

Change to :

    struct Vault {
        address token;
        uint8 premiumIndex; // indexes into `premiumOptions`
        uint8 durationDays; // days
        uint8 dutchAuctionStartingStrikeIndex; // indexes into `strikeOptions`
        uint32 currentExpiration;
        bool isExercised;
        bool isWithdrawing;
        TokenType tokenType;
        uint256 currentStrike;
        uint256 tokenIdOrAmount;
        uint256 dutchAuctionReserveStrike;
    }

2. Title : Saving gas by removing = 0

This code can be saving more gas by removing = 0, it because If a variable was not set/initialized, it is assumed to have default value to 0

##TOOLS USED Manual Review

##Mitigation Step Remove = 0

##Occurance Cally.sol#L94 Cally.sol#L95 Cally.sol#L126

3. Title : using ++i than i++ for saving more gas

This can be using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

4. Title : change uint256 i = 0 into uint256 i for saving more gas

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

Since uint256 default was zero, so this implementation by removing = 0, can saving more gas for each loops.

##Tool Used Manual Review

##Recommended Mitigation Change it