Cally contest - sseefried's results

Lines of code

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

Vulnerability details

Note: This submission contains links to a private fork of the contest repo. User code423n4 has been added as a collaborator in order to view.

Impact

Function getDutchAuctionStrike does not implement the function that Option buyers would expect. They probably expect the curve to decrease smoothly from startingStrike to reserveStrike over the auction period.

While it is true that getDutchAuctionStrike will never return anything less than reserveStrike, whenever reserveStrike is non-zero the curve will decrease to the value of reserveStrike before -- and sometimes well before -- the auction has finished.

The impact is that Call Option sellers potentially don't get as much money as they could have. This is because the curve decreases so quickly down to the reserve strike price.

Proof of Concept

Assume we started the auction at block.timestamp = 0. For the following values

• startStrike = 2.5 ether
• reserveStrike = 1 ether
• block.timestamp = 32000
• auctionEndTimestamp = 86400

you will get a return value of strike = 1 ether even though we are only 32000/86400 = 37.04% through the auction.

The curve of this auction can be seen here. (Make sure to click on the "Execute" button in the top left corner to see graph.) It can also be seen on Imgur.

A Foundry test has been written that also demonstrates the calculation above.

Tools used

Manual inspection

Recommend Mitigation Steps

The code should be changed to

/// @notice Get the current dutch auction strike for a starting strike,
///         reserve strike and end timestamp. Strike decreases quadratically
//          to reserveStrike over time starting at startingStrike. Minimum
//          value returned is reserveStrike.
/// @param startingStrike The starting strike value
/// @param auctionEndTimestamp The unix timestamp when the auction ends
/// @param reserveStrike The minimum value for the strike
/// @return strike The strike
function getDutchAuctionStrike(
    uint256 startingStrike,
    uint32 auctionEndTimestamp,
    uint256 reserveStrike
) public view returns (uint256 strike) {
    /*
        delta = max(auctionEnd - currentTimestamp, 0)
        progress = delta / auctionDuration
        strike = (progress^2 * (startingStrike - reserveStrike)) + reserveStrike
    */
    uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;
    uint256 progress = (1e18 * delta) / AUCTION_DURATION;
    strike = (((progress * progress * (startingStrike - reserveStrike)) / 1e18) / 1e18) + reserveStrike;
}

Note that instead of dividing by 1e18 * 1e18 we divide by 1e18 twice. This leads to better precision. Also, note the use of the word "quadratically" instead of "exponentially" in the documentation of the function.

In colloquial usage the word "exponentially" is often used to mean "really fast" or "non-linear" but it actually has a very specific mathematical meaning. The curve is quadratically decreasing. The reason, from a mathematical standpoint of view, that this is not an exponential curve is that the time value (on the x-axis) is never used as the exponent (i.e. "power") of anything. However it is multiplied by itself, which makes it quadratic.

The curve that the code above generates looks like this. It can also be seen here.

The code has been updated here and another Foundry test has been written that shows it working as expected.

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

Vulnerability details

Note: This submission contains links to a private fork of the contest repo. User code423n4 has been added as a collaborator in order to view.

Impact

In a fully decentralised setting users must also be wary of the smart contract owner and the possibility of "rug pulls". Their trust can be increased by limiting the contract owner's power. In the Cally contract the contract owner is able to set the fee rate arbitrarily high. For values strictly greater than 100% the contract will revert on all calls to exercise leading to a denial of service. However, entering such a value is unlikely and is easily fixed.

However, the fee rate can be set as high at 100% (i.e. 1e18) while still retaining contract functionality. On a call to exercise by an Option holder this means that the vault beneficiary receives no ether! See Cally.sol:289.

Although the feeRate is public, setFee can be called at any time, including after the option has been bought with buyOption.

The impact is that Option sellers only receive the premium but not the strike price. Considering that the strike price is many times larger than the premium this is a substantial loss, and this loss is the contract owner's gain.

Proof of Concept

A Foundry test has been written that shows how the contract owner can rug pull by setting the fee rate to 100%.

The steps are as follows:

• Fee is initially zero
• User babe creates a vault
• User alice buys the option
• User babe sees she got the premium
• Contract owner calls setFee with 1e18 as argument (i.e. 100%)
• User alice exercises the option
• User babe checks her balance and sees that her ethBalance has not increased!
• Contract owner checks protocolUnclaimedFees and sees it has increased by strike price!

Tools Used

Manual inspection

Recommended Mitigation Steps

Add a public constant to the Cally contract that sets a maximum fee rate. For example:

uint256 public constant MAX_FEE_RATE = 5e16; // 0.05%

Then re-implement setFee as follows:

function setFee(uint256 feeRate_) external onlyOwner {
    require(feeRate_ <= MAX_FEE_RATE, "Fee rate is too high!");
    feeRate = feeRate_;
}

An implementation in a private fork appears here and here.

A test confirming that the contract owner can't set a fee rate that is too high appears here.

Also, the function setFee should be called setFeeRate as it is a more accurate name.

Lines of code

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

Vulnerability details

Note: This submission contains links to a private fork of the contest repo. User code423n4 has been added as a collaborator in order to view.

Impact

Function buyOption cannot be called for vault.durationDays >= 195

You get integer overflow for values of 195 and above in the following code.

uint32(block.timestamp) + (vault.durationDays * 1 days)

The reason is subtle and has to do with the "mobile type" associated with number literals.

in Section "Operators" of the Solidity docs it specifies that a literal number is converted to the smallest type that can hold the value. In this case the smallest type that can contain 1 days = 86400 is uint24.

The same section in the Solidity docs then shows us that, for an expression of the form uint8 * uint24, the uint8 is converted (i.e up-cast) to uint24. However, the multiplication still results in an overflow because 195 * 86400 = 16848000 is just bigger than uint24.max = 16777215. However 194 * 86400 = 16761600 and is fine.

This behaviour was so strange to me that I opened an issue in the GitHub Solidity repo which you can read.

The impact of this defect is that Option buyers and sellers will both waste gas. Since calling buyOption always leads to a revert, buyers will waste their gas.

Sellers waste gas because they are forced to initiateWithdraw and withdraw the asset, without having benefited from the sale of the option, because no one can buy the option.

Further, the seller

• may not realise anything has gone wrong and wonder why their Option isn't being bought, and

• realise something has gone wrong but not know why. This is a subtle bug and novices reading the code base may not realise what is going wrong. They may then call createVault again passing in another value for durationDays that is in the range 195 <= durationDays <= 255, thus repeating the mistake.

Proof of Concept

• Call createVault with durationDays equal to 195
• Attempt to call buyOption
• Observe revert due to arithmetic overflow

A Foundry test has been written that exhibits the revert here.

Recommended Mitigation Steps

The fix is to rewrite the expression as

uint32(block.timestamp) + (vault.durationDays * uint32(1 days))

or

uint32(block.timestamp) + (uint32(vault.durationDays) * 1 days)

The latter is preferable.

A demonstration that this code works can be found here and here.

QA Report

Remarks/Recommendations

• the test suite was a joy to work with. I learned a lot about Foundry and will use it on my own projects.
• the documentation was of a very high quality

Low Risk: Don't allow setVaultBeneficiary to be set to address(0)

Yes, getVaultBeneficiary will check for address(0) and return ownerOf[vaultId] but relying on this is a bit dangerous if you change the code in future.

Non-critical: Incorrect error message on check for reserve strike price

There is an incorrect error message in the require at location Cally.sol:169.

It should be:

require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too large");

In general the error message should always express the opposite of the condition of the require statement.

Non-critical: Several requires with == false in them

The locations are

• Cally.sol:217
• Cally.sol:220
• Cally.sol:328

It is better to use the ! (not) operator. e.g.

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