Cally contest - ellahi'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#L283-L286

Vulnerability details

Impact

A malicious owner can keep the feeRate at 1%, but if a user exercises their position, the owner can frontrun the transaction and set the feeRate to 1e18 (100%) and steal all of the vault beneficiary's funds.

Proof of Concept

1. Alice creates vault with 0.1 ETH premium, 30 day duration on her BAYC, max strike of 500 ETH
2. Dutch auction starts at a strike of 500 ETH and decreases to 0 ETH over a 24 hour auction period
3. Bob buys the call for 0.1 ETH, 30 day duration at a strike of 164.3 ETH after T amount of time has passed
4. After 23 days, Bob wants to exercise his option and calls exercise() sending 164.3 ETH to the contract.
5. The malicious owner sees Bob's transaction enter the mempool and frontruns it, calling the setFee() function and sets the feeRate to equal 100%.

/// @notice Sets the fee that is applied on exercise
/// @param feeRate_ The new fee rate: fee = 1% = (1 / 100) * 1e18
function setFee(uint256 feeRate_) external onlyOwner {
    feeRate = feeRate_;
}

6. Bob's tx goes through after the owners, and now fee effectively equals msg.value.

// collect protocol fee
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;

msg.value = 164.3 ETH. fee = (164.3 ETH * 1e18) / 1e18 = 164.3 ETH. protocolUnclaimedFees += 164.3 ETH. ethBalance[getVaultBeneficiary(vaultId)] += 0.

7. Alice's, the vault beneficiary, ethBalance doesn't increment since msg.value - fee = 0.
8. After Bob sends 164.3 ETH (strike amount) to the contract, the BAYC is sent to him. The 164.3 ETH is credited to Alice the protocol's unclaimed fees.
9. Malicious owner calls withdrawProtocolFees() and has effectively stolen 164.3 ETH.
10. Alice is now sad she no longer has her NFT and the 164.3 ETH she was supposed to be credited with.

In reality, the owner does not need to frontrun any transaction. The feeRate can be changed at any time and go unnoticed until it is too late for affected user(s) to realize what has happened.

Tools Used

Manual.

Recommended Mitigation Steps

To increase community trust, it is suggested to add an upper bound when setting the new feeRate (like 5% max fee), and it may be ideal to add a timelock on top of it as well.

[L-01] Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Proof of Concept

Recommendation

Consider adding a pendingOwner where the new owner will have to accept the ownership.

address owner;
address pendingOwner;

// ...

function setPendingOwner(address newPendingOwner) external {
    require(msg.sender == owner, "!owner");
    emit NewPendingOwner(newPendingOwner);
    pendingOwner = newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner, "!pendingOwner");
    emit NewOwner(pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

[L-02] Override Ownable.sol functions.

Impact

Currently, the function transferOwnership and renounceOwnership can be called by the owner in Cally.sol. Both functions are a one-step process, and can be called accidentally, resulting in loss of ownership. All protocol fee's would get stuck if this is the case.

Recommendation

Override the functions to avoid such scenario.

[L-03] Unclear documentation

Cally.sol#L359-L369. Check @audit tag.

/// @notice Sends any unclaimed ETH (premiums/strike) to the msg.sender
function harvest() public returns (uint256 amount) {
    // reset premiums
    amount = ethBalance[msg.sender];
    ethBalance[msg.sender] = 0;

    emit Harvested(msg.sender, amount);

    // transfer premiums to owner // @audit which owner? Unclear, be more specific.
    payable(msg.sender).safeTransferETH(amount);
}

Comment may result in confusion. It is probably meant to say ...to msg.sender or ... to vault owner.

Recommendation

Fix comment.

Non-Critical

[N-01] Natspec is incomplete

Cally.sol#L123-L124

/// @notice Withdraws the protocol fees and sends to current owner
function withdrawProtocolFees() external onlyOwner returns (uint256 amount) {

Cally.sol#L157-L166

/// @param tokenType The type of the underlying asset (NFT or ERC20)
function createVault(
    uint256 tokenIdOrAmount,
    address token,
    uint8 premiumIndex,
    uint8 durationDays,
    uint8 dutchAuctionStartingStrikeIndex,
    uint256 dutchAuctionReserveStrike,
    TokenType tokenType
) external returns (uint256 vaultId) {

Cally.sol#L206-L207

/// @param vaultId The tokenId of the vault to buy the option from
function buyOption(uint256 vaultId) external payable returns (uint256 optionId) {

Cally.sol#L359-L360

/// @notice Sends any unclaimed ETH (premiums/strike) to the msg.sender
function harvest() public returns (uint256 amount) {

Cally.sol#L386-L387

/// @param vaultId The tokenId of the vault to fetch the details for
function vaults(uint256 vaultId) external view returns (Vault memory) {

Recommendation

Add @return's.

Tools used

manual, slither

[N-02] constants should be defined rather than using magic numbers.

  Cally.sol::284 => fee = (msg.value * feeRate) / 1e18;
  Cally.sol::418 => uint256 progress = (1e18 * delta) / AUCTION_DURATION;
  Cally.sol::419 => uint256 auctionStrike = (progress * progress * startingStrike) / (1e18 * 1e18);

[G-01] Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  CallyNft.sol::244 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Store the array’s length in a variable before the for-loop.

[G-02] Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

  Cally.sol::170 => require(durationDays > 0, "durationDays too small");

Recommendation

Use != 0 instead of > 0.

[G-03] Use Custom Errors instead of Revert Strings.

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Recommendation

Use custom errors instead of revert strings.

[G-04] No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  Cally.sol::94 => uint256 feeRate = 0;
  Cally.sol::95 => uint256 protocolUnclaimedFees = 0;  
  Cally.sol::282 => uint256 fee = 0;
  CallyNft.sol::244 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Remove explicit zero initialization.

[G-05] ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Proof of Concept

  CallyNft.sol::244 => for (uint256 i = 0; i < data.length; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-06] Use Shift Right/Left instead of Division/Multiplication whenever possible

Impact

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

Proof of Concept

  CallyNft.sol::241 => bytes memory str = new bytes(2 + data.length * 2);
  CallyNft.sol::245 => str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))];
  CallyNft.sol::246 => str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))];
  base64.sol::78 => uint256 decodedLen = (data.length / 4) * 3;

Bad

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

Good

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Recommendation

Use SHR/SHL.

Tools used

c4udit, manual, slither