Cally contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

setFee allow the owner to see feeRate to any value, this lead to 2 problems.

1. If feeRate > 1e18, user will not be able to exercise options as L289 would revert due to underflow.
2. Owner can set feeRate to 100% to rug user

Proof of Concept

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#L282-L289

        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;

Recommended Mitigation Steps

Add a check to make sure feeRate is below some constant value e.g. 10%

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

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

        require(durationDays > 0, "durationDays too small");

No need to check zero address if its returning uint.max anyway

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

        require(owner != address(0), "ZERO_ADDRESS");

Consider to use a polynomial instead of storing constants in storage

Consider to use a polynomial instead of storing premiumOptions and strikeOptions can save gas by reducing SLOADS https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L89-L93

    // prettier-ignore
    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];

Unchecked increment

This line will never overflow, we can use unchecked to save gas

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

        vaultIndex += 2;

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

        optionId = vaultId + 1;

Unnecessary ops with vaultId

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

        vaultIndex += 2;
        vaultId = vaultIndex;
        _vaults[vaultId] = vault;

change into

        vaultId = (vaultIndex += 2);
        _vaults[vaultId] = vault;

Unnecessary ops with optionId

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

        uint256 optionId = vaultId + 1;
        _burn(optionId);
        _burn(vaultId);

change into

        _burn(vaultId + 1);
        _burn(vaultId);

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/

Float multiplication optimization

We can use the following function to save gas on float multiplications

// out = x * y unchecked{/} z
function fmul(uint256 x, uint256 y, uint256 z) internal pure returns(uint256 out){
assembly{
if iszero(eq(div(mul(x,y),x),y)) {revert(0,0)}
out := div(mul(x,y),z)
}
}

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

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

Use delete instead of setting to 0

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

            _vaultBeneficiaries[id] = address(0);