Platform: Code4rena
Start Date: 10/05/2022
Pot Size: $50,000 USDC
Total HM: 13
Participants: 100
Period: 5 days
Judge: HardlyDifficult
Total Solo HM: 1
Id: 122
League: ETH
Rank: 17/100
Findings: 1
Award: $621.88
🌟 Selected for report: 1
🚀 Solo Findings: 0
621.8845 USDC - $621.88
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
Affected code:
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:
So the vault will be unusable and the ETH deposited by users to buy/exercise options will remain locked in Cally contract
token and 13 as tokenIdOrAmountcreateVault() 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 runs13 is pretty limited in functionality, because Cally’ smart contract is the ownerexercise 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 foreverEditor
Add the following check at the start of createVault():
require(token != address(this), "Cant use Cally as token");
#0 - outdoteth
2022-05-16T09:40:01Z
This is an exploit that requires users to actively make a very precise and niche mistake. should be medium severity in our opinion.
#1 - outdoteth
2022-05-17T17:36:57Z
fix for this issue is here: https://github.com/outdoteth/cally/pull/10
#2 - HardlyDifficult
2022-05-20T15:32:49Z
Copying in the POC from GimelSec in #244 because it's an interesting attack to consider for this issue as well.
createVault to vault a NFT which worth 100 ETH, with parameters:
dutchAuctionStartingStrikeIndex is set to 0 (which strikeOptions is 1 ETH)durationDays, e.g. 255 daysvaultId 1 token, and Alice do another transaction: call buyOption(1) to get a optionId 2 tokenvaultId 1 token with strike 89 ETH, and get a vaultId 3 tokenvaultId 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).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.