Cally contest - RagePit's results

Lines of code

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

Vulnerability details

Impact

Owner setting fee rate above 1e18 will dos any exercise() calls.

Proof of Concept

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;

if feeRate is set to higher than 1e18, fee will be greater than msg.value. This will result in underflow on line 289 and any call to exercise will revert. This will result in a permanent loss of option premiums with no option to exercise if the feeRate stays above 1e18. Assumes a malicious owner.

Recommended Mitigation Steps

Add a cap to feeRate

function setFee(uint256 feeRate_) external onlyOwner {
	require(feeRate_ <= 1e18, "Invalid Fee Rate");
	feeRate = feeRate_;
}

Gas Optimizations

Extra vaultIndex SLOAD

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

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

Saves an erreneous SLOAD of vaultIndex. Saves 113 gas per createVault call

transferFrom gas savings

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

_vaultBeneficiaries[id] = address(0);

An option nft will never have a vault beneficiary, so the isVaultToken check is pointless. Saves about 25 gas in the case of a vault nft id being transferred.

getPremium savings

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

return premiumOptions[_vaults[vaultId].premiumIndex];

Even though it's a view function, saving the entire Vault struct to memory is expensive. Will save gas on composable contracts.

createVault get vault data from calldata variables

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L173 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L198-L200 No need to store the Vault struct in memory. All of the vault data is sent in the calldata anyway

Recommendation

_vaults[vaultId] = Vault({...});
...
tokenType == TokenType.ERC721
	? ERC721(token).transferFrom(msg.sender, address(this), tokenIdOrAmount)
	: ERC20(token).safeTransferFrom(msg.sender, address(this), tokenIdOrAmount);

Reduce Vault struct storage size

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L83-L84 currentStrike and dutchAuctionReserveStrike members of Vault struct are unnecessarily large. The max strike price is 6765 ether or 73 bits.

Recommendation: Change currentStrike and dutchAuctionReserveStrike to be between uint80-uint128. This brings the Vault struct to 3 storage slots rather than 4. Changing both to uint128 saves about 2100 gas per call to createVault, exercise, withdraw or buyOption (nearly 2%)