It shouldn’t be possible to create a vault with Cally’ own token

[code423n4]

Lines of code

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

Vulnerability details

Impact

Affected code:

• https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L193
• https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L199

Currently it’s possible to create an ERC-721 vault using Cally’ own address as token, and using the freshly minted vault id as tokenIdOrAmount. This results in a new vault whose ownership is passed to Cally contract immediately upon creation.

The vault allows users to perform buyOption and increase the ETH balance of the Cally contract itself, which is still the vault beneficiary. As soon as an user calls exercise, she will receive the vault.tokenIdOrAmount in exchange, which in this case coincides with the vault nft. However this is of no good because the final user may just initiate a withdrawal, which will:

• always fail because the vault id is burned (https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L335) and then transferred back to the user (https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L344)
• leave all the ETH unredemable in Cally contract

So the vault will be unusable and the ETH deposited by users to buy/exercise options will remain locked in Cally contract

Proof of Concept

• Current vault id is, let’s say, 11
• User deploys a vault with Cally’ address as token and 13 as tokenIdOrAmount
• Since createVault() mints the vault token to the user, and then transfers the underlying address from the user, an user is able to create a vault with something she doesn’t own at the moment of the createVault() function call, because it’s created while the function runs
• The vault 13 is pretty limited in functionality, because Cally’ smart contract is the owner
• However, users can still buy options: so Alice and Bob deposit their premiums
• Whoever exercise the active option, becomes the vault owner now; this is of no good because no one can actually call withdraw() as it will always revert, and no one can recover the ETH deposited by Alice and Bob as they are locked forever

Tools Used

Editor

Recommended Mitigation Steps

Add the following check at the start of createVault():

require(token != address(this), "Cant use Cally as token");

[outdoteth]

This is an exploit that requires users to actively make a very precise and niche mistake. should be medium severity in our opinion.

[outdoteth]

fix for this issue is here: outdoteth/cally#10

[HardlyDifficult]

Copying in the POC from GimelSec in #244 because it's an interesting attack to consider for this issue as well.

1. Alice (Attacker) pack 2 transactions into same block:
   • first transaction: calls createVault to vault a NFT which worth 100 ETH, with parameters:
     • dutchAuctionStartingStrikeIndex is set to 0 (which strikeOptions is 1 ETH)
     • a long durationDays, e.g. 255 days
   • then Alice will get a vaultId 1 token, and Alice do another transaction: call buyOption(1) to get a optionId 2 token
2. Alice re-vault the vaultId 1 token with strike 89 ETH, and get a vaultId 3 token
3. Bob see that the auction of vaultId 3 token is 89 ETH, but the vaultId 3 token can get the NFT which worth 100 ETH, so Bob pays 89 ETH, calls buyOption(3), and exercise(4) to get the vaultId 1 token. Then, Bob calls initiateWithdraw(1) and waits for the optionId 2 token to expire (which durationDays is set to 255 days in step 1).
4. Alice monitors that someone bought the vaultId 1 token, then Alice quickly calls exercise(2). Finally, Alice just pays Bob 1 ETH, and gets the NFT back. Alice also gets 89 ETH which is paid by Bob from the vaultId 3 token.

I agree with (2) Medium for this issue. It can be abused, but the impacted parties can clearly see this is an attempt to subvert the system in some way (a vault of a vault with an NFT, instead of a single value with an NFT as expected). That should be a red flag for Bob in the example above.