There is no timelock on setFee(). This is the fee that is applied in exercise(), and determines how much the Vault Creator is actually credited upon a call option being exercised. But:
there is no maximum value limiting what fee can be set to
there is no timelock, ie feeRate is set to its new value when setFee() is created.
Users will be incited to create vaults if the fee is low (feeRate is initially 0). A malicious owner could effectively wait for vaults being created and call options to be bought, then set a very high fee, which would result in any exercised call option sending all the strike ETH to the Cally owner.
Impact
Medium
Proof Of Concept
Let's take a similar example as in the Readme file:
Alice creates a vault with 0.1 ETH premium, 30 day duration on her BAYC, max strike of 500 ETH
Dutch auction starts at a strike of 500 ETH and decreases to 0 ETH over a 24 hour auction period
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
The malicious owner now calls setFee(1e18), setting feeRate as 100%.
Bob exercises his option after 23 days
The BAYC is sent to Bob. But because of the new fee, and of this part of the code, the strike amount is collected as protocol fees, and Alice's ETH balance does not get increased. Alice has effectively lost her BAYC.
Tools Used
Manual Analysis
Recommended Mitigation Steps
Two things can be done:
set a maximum possible fee rate in setFee()
add a timelock in setFee(), ensuring all active options are not affected by the change in feeRate.
Lines of code
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L119 https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L283-L285
Vulnerability details
NO TIMELOCK ON
SETFEE()
There is no timelock on
setFee()
. This is the fee that is applied inexercise()
, and determines how much the Vault Creator is actually credited upon a call option being exercised. But:feeRate
is set to its new value whensetFee()
is created.Users will be incited to create vaults if the fee is low (
feeRate
is initially0
). A malicious owner could effectively wait for vaults being created and call options to be bought, then set a very high fee, which would result in any exercised call option sending all the strike ETH to the Callyowner
.Impact
Medium
Proof Of Concept
Let's take a similar example as in the Readme file:
T
amount of time has passedThe malicious owner now calls
setFee(1e18)
, settingfeeRate
as 100%.The BAYC is sent to Bob. But because of the new fee, and of this part of the code, the strike amount is collected as protocol fees, and Alice's ETH balance does not get increased. Alice has effectively lost her BAYC.
Tools Used
Manual Analysis
Recommended Mitigation Steps
Two things can be done:
setFee()
setFee()
, ensuring all active options are not affected by the change infeeRate
.