| title | sponsor | slug | date | findings | contest |
|---|---|---|---|---|---|
Cally contest |
Cally |
2022-05-cally |
2022-07-01 |
122 |
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the Cally smart contract system written in Solidity. The audit contest took place between May 10—May 14 2022.
115 Wardens contributed reports to the Cally contest:
- smiling_heretic
- hubble (ksk2345 and shri4net)
- sseefried
- IllIllI
- 0xDjango
- Ruhum
- Hawkeye (0xwags and 0xmint)
- oyc_109
- GimelSec (rayn and sces60107)
- BondiPestControl (leastwood and kirk-baird)
- WatchPug (jtp and ming)
- 0xsanson
- antonttc
- VAD37
- horsefacts
- TrungOre
- p4st13r4 (0x69e8 and 0xb4bb4)
- hickuphh3
- shung
- MiloTruck
- BowTiedWardens (BowTiedHeron and BowTiedPickle and m4rio_eth and Dravee and BowTiedFirefox)
- MaratCerby
- hake
- ellahi
- joestakey
- dipp
- 0xf15ers (remora and twojoy)
- shenwilly
- Kenshin
- FSchmoede
- cccz
- minhquanym
- Bludya
- catchup
- rfa
- Cityscape
- Picodes
- reassor
- pedroais
- robee
- pmerkleplant
- rotcivegaf
- TomFrenchBlockchain
- csanuragjain
- djxploit
- 0x4non
- CertoraInc (egjlmn1, OriDabush, ItayG, and shakedwinder)
- 0x1f8b
- fatherOfBlocks
- sikorico
- Waze
- z3s
- bobirichman
- delfin454000
- hansfriese
- mics
- Funen
- Kumpa
- jah
- berndartmueller
- 0x1337
- Czar102
- eccentricexit
- MadWookie
- hyh
- seanamani
- dirk_y
- radoslav11
- sorrynotsorry
- Aits
- 242
- cryptphi
- BouSalman
- AlleyCat
- JDeryl
- defsec
- RagePit
- jayjonah8
- m9800
- gzeon
- throttle
- _Adam
- TerrierLover
- 0xkatana
- samruna
- 0xNazgul
- simon135
- DavidGialdi
- ignacio
- Tadashi
- 0v3rf10w
- Fitraldys
- jonatascm
- peritoflores
- 0x52
- ACai
- kebabsec (okkothejawa and FlameHorizon)
- ynnad
- PPrieditis
- crispymangoes
This contest was judged by HardlyDifficult.
Final report assembled by itsmetechjay.
The C4 analysis yielded an aggregated total of 13 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 10 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 67 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 59 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 Cally contest repository, and is composed of 2 smart contracts written in the Solidity programming language and includes 439 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
Submitted by Ruhum, also found by oyc_109 and smiling_heretic
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L198-L200
Some ERC20 tokens don't throw but just return false when a transfer fails. This can be abused to trick the createVault() function to initialize the vault without providing any tokens. A good example of such a token is ZRX: Etherscan code
When such a vault is initialized, another user can both buy and exercise the option without ever receiving any funds. The creator of the vault does receive the buyer's Ether tho. So it can cause a loss of funds.
The trick is to create a vault with an ERC20 token but use ERC721 as the vault's type. Then, instead of calling safeTransferFrom() the function calls transferFrom() which won't catch the token returning false.
Here's a test that showcases the issue:
// CreateVault.t.sol
function testStealFunds() public {
// address of 0x on mainnet
address t = address(0xE41d2489571d322189246DaFA5ebDe1F4699F498);
vm.startPrank(babe);
require(ERC20(t).balanceOf(babe) == 0);
uint vaultId = c.createVault(100, t, 1, 1, 1, 0, Cally.TokenType.ERC721);
// check that neither the Cally contract nor the vault creator
// had any 0x tokens
require(ERC20(t).balanceOf(babe) == 0);
require(ERC20(t).balanceOf(address(c)) == 0);
// check whether vault was created properly
Cally.Vault memory v = c.vaults(vaultId);
require(v.token == t);
require(v.tokenIdOrAmount == 100);
vm.stopPrank();
// So now there's a vault for 100 0x tokens although the Cally contract doesn't
// have any.
// If someone buys & exercises the option they won't receive any tokens.
uint premium = 0.025 ether;
uint strike = 2 ether;
require(address(c).balance == 0, "shouldn't have any balance at the beginning");
require(payable(address(this)).balance > 0, "not enough balance");
uint optionId = c.buyOption{value: premium}(vaultId);
c.exercise{value: strike}(optionId);
// buyer of option (`address(this)`) got zero 0x tokens
// But buyer lost their Ether
require(ERC20(t).balanceOf(address(this)) == 0);
require(address(c).balance > 0, "got some money");
}To run it, you need to use forge's forking mode: forge test --fork-url <alchemy/infura URL> --match testStealFunds
I think the easiest solution is to use safeTransferFrom() when the token is of type ERC721. Since the transfer is at the end of the function there shouldn't be any risk of reentrancy. If someone passes an ERC20 address with type ERC721, the safeTransferFrom() call would simply fail since that function signature shouldn't exist on ERC20 tokens.
outdoteth (Cally) confirmed and resolved:
the fix for this issue is here; outdoteth/cally#4
HardlyDifficult (judge) commented:
This is a great report. I appreciate the clear test showcasing the issue well, and using a real token example.
Submitted by hubble, also found by Hawkeye and sseefried
The vulnerability or bug is in the implementation of the function getDutchAuctionStrike() The AUCTION_DURATION is defined as 24 hours, and consider that the dutchAuctionReserveStrike (or reserveStrike) will never be set to 0 by user.
Now if a vault is created with startingStrike value of 55 and reserveStrike of 13.5 , the auction price will drop from 55 to 13.5 midway at ~12 hours. So, after 12 hours from start of auction, the rate will be constant at reserveStrike of 13.5, and remaining time of 12 hours of auction is a waste.
Some other examples :
startStrike, reserveStrike, time-to-reach-reserveStrike
55 , 13.5 , ~12 hours
55 , 5 , ~16.7 hours
55 , 1.5 , ~20 hours
5 , 1.5 , ~11 hours
The impact is high wrt Usability, where users have reduced available time to participate in the auction (when price is expected to change). The vault-Creators or the option-Buyers may or may not be aware of this inefficiency, i.e., how much effective time is available for auction.
Contract : Cally.sol Function : getDutchAuctionStrike ()
The function getDutchAuctionStrike() can be modified such that price drops to the reserveStrike exactly at 24 hours from start of auction.
/*
delta = max(auctionEnd - currentTimestamp, 0)
progress = delta / auctionDuration
auctionStrike = progress^2 * (startingStrike - reserveStrike) << Changes here
strike = auctionStrike + reserveStrike << Changes here
*/
uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;
uint256 progress = (1e18 * delta) / AUCTION_DURATION;
uint256 auctionStrike = (progress * progress * (startingStrike-reserveStrike)) / (1e18 * 1e18);
strike = auctionStrike + reserveStrike;
outdoteth (Cally) confirmed, disagreed with severity and commented:
We think this should be bumped to high severity. It would be easy for a user to create an auction that declines significantly faster than what they would have assumed - even over 1 or 2 blocks. It makes no sense for the auction to ever behave in this way and would result in options getting filled at very bad prices for the creator of the vault.
The fix for this issue is here: outdoteth/cally#2
HardlyDifficult (judge) increased severity to High and commented:
The sponsor comment here makes sense. Agree with (1) High since this can potentially be very detrimental to the promise of this protocol.
[H-03] [WP-H0] Fake balances can be created for not-yet-existing ERC20 tokens, which allows attackers to set traps to steal funds from future users
Submitted by WatchPug, also found by 0xsanson, BondiPestControl, and IllIllI
function createVault(
uint256 tokenIdOrAmount,
address token,
...
) external returns (uint256 vaultId) {
...
Vault memory vault = Vault({
...
});
// vault index should always be odd
vaultIndex += 2;
vaultId = vaultIndex;
_vaults[vaultId] = vault;
// give msg.sender vault token
_mint(msg.sender, vaultId);
emit NewVault(vaultId, msg.sender, token);
// transfer the NFTs or ERC20s to the contract
vault.tokenType == TokenType.ERC721
? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
: ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
}import "solmate/utils/SafeTransferLib.sol";
...
contract Cally is CallyNft, ReentrancyGuard, Ownable {
using SafeTransferLib for ERC20;
...When creating a new vault, solmate's SafeTransferLib is used for pulling vault.token from the caller's account, this issue won't exist if OpenZeppelin's SafeERC20 is used instead.
That's because there is a subtle difference between the implementation of solmate's SafeTransferLib and OZ's SafeERC20:
OZ's SafeERC20 checks if the token is a contract or not, solmate's SafeTransferLib does not.
See: https://github.com/Rari-Capital/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.
As a result, when the token's address has no code, the transaction will just succeed with no error.
This attack vector was made well-known by the qBridge hack back in Jan 2022.
For our project, this alone still won't be a problem, a vault created and wrongfully accounted for a certain amount of balance for a non-existing token won't be much of a problem, there will be no fund loss as long as the token stays that way (being non-existing).
However, it's becoming popular for protocols to deploy their token across multiple networks and when they do so, a common practice is to deploy the token contract from the same deployer address and with the same nonce so that the token address can be the same for all the networks.
For example: $1INCH is using the same token address for both Ethereum and BSC; Gelato's $GEL token is using the same token address for Ethereum, Fantom and Polygon.
A sophisticated attacker can exploit it by taking advantage of that and setting traps on multiple potential tokens to steal from the future users that deposits with such tokens.
Given:
- ProjectA has TokenA on another network;
- ProjectB has TokenB on another network;
- ProjectC has TokenC on another network;
- The attacker
createVault()forTokenA,TokenB, andTokenCwith10000e18astokenIdOrAmounteach; - A few months later, ProjectB lunched
TokenBon the local network at the same address; - Alice created a vault with
11000e18 TokenB; - The attacker called
initiateWithdraw()and thenwithdraw()to receive10000e18 TokenB.
In summary, one of the traps set by the attacker was activated by the deployment of TokenB and Alice was the victim. As a result, 10000e18 TokenB was stolen by the attacker.
Consider using OZ's SafeERC20 instead.
outdoteth (Cally) confirmed and resolved:
this issue has been fixed here: outdoteth/cally#5
HardlyDifficult (judge) commented:
Great catch and the potential attack is very clearly explained. Although the window for an attack like this would not be common, it's an easy trap to setup and likely would occur as some point if Cally is planning to support multiple networks.
Submitted by IllIllI, also found by Adam, 0x52, 0xf15ers, 0xsanson, berndartmueller, Bludya, BondiPestControl, catchup, crispymangoes, Czar102, eccentricexit, ellahi, GimelSec, hake, horsefacts, hubble, joestakey, Kumpa, pedroais, peritoflores, reassor, shenwilly, shung, smiling_heretic, sseefried, and throttle
Owner can steal the exercise cost which should have gone to the option seller
There are no restrictions on when the owner can set the feeRate:
File: contracts/src/Cally.sol #1
117 /// @notice Sets the fee that is applied on exercise
118 /// @param feeRate_ The new fee rate: fee = 1% = (1 / 100) * 1e18
119 function setFee(uint256 feeRate_) external onlyOwner {
120 feeRate = feeRate_;
121 }By using a rate that consumes the exercise cost, the owner can steal Ether from the seller:
File: contracts/src/Cally.sol #2
282 uint256 fee = 0;
283 if (feeRate > 0) {
284 fee = (msg.value * feeRate) / 1e18;
285 protocolUnclaimedFees += fee;
286 }
287
288 // increment vault beneficiary's ETH balance
289 ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;The owner can wait for a particularly large-value NFT, snipe that one option, then retire
Fix the fee rate per vault during vault creation
outdoteth (Cally) confirmed and resolved:
issue is fixed here: outdoteth/cally#8
HardlyDifficult (judge) commented:
This is a valid concern. The owner could abuse this to capture much more ETH than was intended. It assumes that the owner is malicious or compromised and does not introduce any more risk than an upgradeable contract would have.
Submitted by p4st13r4, also found by GimelSec, TrungOre, and VAD37
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:
- 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
- Current vault id is, let’s say, 11
- User deploys a vault with Cally’ address as
tokenand13astokenIdOrAmount - 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 thecreateVault()function call, because it’s created while the function runs - The vault
13is 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
exercisethe active option, becomes the vault owner now; this is of no good because no one can actually callwithdraw()as it will always revert, and no one can recover the ETH deposited by Alice and Bob as they are locked forever
Add the following check at the start of createVault():
require(token != address(this), "Cant use Cally as token");outdoteth (Cally) confirmed, disagreed with severity and commented:
This is an exploit that requires users to actively make a very precise and niche mistake. should be medium severity in our opinion.
fix for this issue is here: outdoteth/cally#10
HardlyDifficult (judge) decreased severity to Medium and commented:
Copying in the POC from GimelSec in #244 because it's an interesting attack to consider for this issue as well.
- Alice (Attacker) pack 2 transactions into same block:
- first transaction: calls
createVaultto vault a NFT which worth 100 ETH, with parameters:
dutchAuctionStartingStrikeIndexis set to 0 (whichstrikeOptionsis 1 ETH)- a long
durationDays, e.g.255days- then Alice will get a
vaultId 1token, and Alice do another transaction: callbuyOption(1)to get aoptionId 2token- Alice re-vault the
vaultId 1token with strike 89 ETH, and get avaultId 3token- Bob see that the auction of
vaultId 3token is 89 ETH, but thevaultId 3token can get the NFT which worth 100 ETH, so Bob pays 89 ETH, callsbuyOption(3), andexercise(4)to get thevaultId 1token. Then, Bob callsinitiateWithdraw(1)and waits for theoptionId 2token to expire (whichdurationDaysis set to255days in step 1).- Alice monitors that someone bought the
vaultId 1token, then Alice quickly callsexercise(2). Finally, Alice just pays Bob 1 ETH, and gets the NFT back. Alice also gets 89 ETH which is paid by Bob from thevaultId 3token.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.
[M-03] User's may accidentally overpay in buyOption() and the excess will be paid to the vault creator
Submitted by BondiPestControl, also found by 0xf15ers, berndartmueller, cccz, csanuragjain, dipp, GimelSec, hake, horsefacts, IllIllI, jayjonah8, m9800, MadWookie, MiloTruck, pedroais, Ruhum, throttle, and VAD37
It is possible for a user purchasing an option to accidentally overpay the premium during buyOption().
Any excess funds paid for in excess of the premium will be transferred to the vault creator.
The premium is fixed at the time the vault is first created by vault.premiumIndex. Hence there is no need to allow users to overpay since there will be no benefit.
buyOption() allows msg.value > premium
uint256 premium = getPremium(vaultId);
require(msg.value >= premium, "Incorrect ETH amount sent");Consider modifying the check such that the msg.value is exactly equal to the premuim. e.g.
uint256 premium = getPremium(vaultId);
require(msg.value == premium, "Incorrect ETH amount sent");outdoteth (Cally) confirmed and resolved:
this issue is fixed in: outdoteth/cally#9
HardlyDifficult (judge) commented:
Agree with 2 (Medium) for this. The issue doesn't really open the door for an attack, except for maybe via a malicious frontend. But it could potentially leak value in terms of over compensating the vault creator.
QA report #182 should have its issue bumped up and marked as a duplicate IMO
Submitted by IllIllI, also found by horsefacts and smiling_heretic
Rebasing tokens are tokens that have each holder's balanceof() increase over time. Aave aTokens are an example of such tokens.
If rebasing tokens are used as the vault token, rewards accrue to the vault and cannot be withdrawn by either the option seller or the owner, and remain locked forever.
The amount 'available' for withdrawal comes from an input parameter and is stored for later operations:
File: contracts/src/Cally.sol #1
173 Vault memory vault = Vault({
174 tokenIdOrAmount: tokenIdOrAmount,
175 token: token,
176 premiumIndex: premiumIndex,
177 durationDays: durationDays,
178 dutchAuctionStartingStrikeIndex: dutchAuctionStartingStrikeIndex,
179 currentExpiration: uint32(block.timestamp),
180 isExercised: false,
181 isWithdrawing: false,
182 tokenType: tokenType,
183 currentStrike: 0,
184 dutchAuctionReserveStrike: dutchAuctionReserveStrike
185 });
186
187 // vault index should always be odd
188 vaultIndex += 2;
189 vaultId = vaultIndex;
190 _vaults[vaultId] = vault;
191
192 // give msg.sender vault token
193 _mint(msg.sender, vaultId);
194
195 emit NewVault(vaultId, msg.sender, token);
196
197 // transfer the NFTs or ERC20s to the contract
198 vault.tokenType == TokenType.ERC721
199 ? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
200 : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);The amount actually available grows over time and is only known at the time of withdrawal. The option withdrawal/exercise use the original amount:
File: contracts/src/Cally.sol #2
345 : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);File: contracts/src/Cally.sol #3
296 : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);Track total amounts currently deposited and allow vault creators to withdraw excess on a pro-rata basis
outdoteth (Cally) acknowledged and commented:
This is technically an issue however we have no intention of supporting rebase tokens
Submitted by hickuphh3, also found by BondiPestControl, GimelSec, sseefried, and VAD37
vault.durationDays is of type uint8, thus allowing a maximum value of 255. 1 days = 86400, thus fitting into a uint24. Solc creates a temporary variable to hold the result of the intermittent multiplication vault.durationDays * 1 days using the data type of the larger operand.
In this case, the intermittent data type used would be uint24, which has a maximum value of 2**24 - 1 = 16777215. The maximum number allowable before overflow achieved is therefore (2**24 - 1) / 86400 = 194.
Insert this test case into BuyOption.t.sol
function testCannotBuyDueToOverflow() public {
vm.startPrank(babe);
bayc.mint(babe, 2);
// duration of 195 days
vaultId = c.createVault(2, address(bayc), premiumIndex, 195, strikeIndex, 0, Cally.TokenType.ERC721);
vm.stopPrank();
vm.expectRevert(stdError.arithmeticError);
c.buyOption{value: premium}(vaultId);
}Then run
forge test --match-contract TestBuyOption --match-test testCannotBuyDueToOverflow
This was the 1 high-severity bug that I wanted to mention at the end of the C4 TrustX showcase but unfortunately could not due to a lack of time :( It can be found in the vulnerable lottery contract on L39. Credits to Pauliax / Thunder for the recommendation and raising awareness of this bug =p
Cast the multiplication into uint32.
vault.currentExpiration = uint32(block.timestamp) + uint32(vault.durationDays) * 1 days;outdoteth (Cally) confirmed and commented:
Agree that this is high risk - a user can unintentionally create vaults that would never have been able to have been filled and result in them losing funds because the vault creation was useless. They then also have to initiate a withdraw and then actually withdraw before they can create another vault.
In terms of gas prices at 100 gwei (which it frequently was a few months ago) the total gas cost of this bug/incorrect vault creation is not insignificant.
This issue is fixed here; outdoteth/cally#3
HardlyDifficult (judge) decreased severity to Medium and commented:
This is an easy way someone could create a vault where it's not possible to buy an option, and without using an unreasonably high duration value. If this were to occur, the vault creator could immediately
initiateWithdrawand thenwithdraw. No time delay is required and the only funds lost is the gas cost of those 3 transactions.Lowering to 2 (Medium) since there's an easy recovery and no assets lost.
[M-06] Owner can set the feeRate to be greater than 100% and cause all future calls to exercise to revert
Submitted by IllIllI, also found by 0xDjango, ACai, antonttc, BowTiedWardens, Cityscape, defsec, dipp, FSchmoede, GimelSec, gzeon, hickuphh3, hubble, joestakey, Kenshin, m9800, MiloTruck, RagePit, Ruhum, shenwilly, TomFrenchBlockchain, and WatchPug
The owner can force options to be non-exercisable, collecting premium without risking the loss of their NFT/tokens
After a buyer buys an option owned by the owner, the owner can change the fee rate to be close to type(uint256).max, which will cause the subtraction below to always underflow, preventing the exercise of the option. Once the option expires, the owner can change the fee back and wait for another buyer
File: contracts/src/Cally.sol #1
288 // increment vault beneficiary's ETH balance
289 ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;Add reasonable fee rate bounds checks in the setFee() function
outdoteth (Cally) confirmed and resolved:
this issue is fixed here; outdoteth/cally#7
Submitted by 0xDjango
A griefer is able to create as many vaults as they want by simply calling createVault() with tokenIdOrAmount = 0. This will most likely pose problems on the front-end of the Cally protocol because there will be a ridiculously high number of malicious vaults displayed to actual users.
I define these vaults as malicious because it is possible that a user accidently buys a call on this vault which provides 0 value in return. Overall, the presence of zero-amount vaults is damaging to Cally's product image and functionality.
- User calls
createVault(0,,,,);with an ERC20 type. - There is no validation that
amount > 0 - Function will complete successfully, granting the new vault NFT to the caller.
- Cally protocol is filled with unwanted 0 amount vaults.
Add the simple check require(tokenIdOrAmount > 0, "Amount must be greater than 0");
outdoteth (Cally) confirmed and commented:
This check should only be applied on ERC20 tokens because ERC721 tokens can still have tokenIds that have ID's with a value of 0.
this issue is fixed here: outdoteth/cally#12
Submitted by 0x1337, also found by 0x52, 0xDjango, 0xsanson, berndartmueller, BondiPestControl, BowTiedWardens, cccz, dipp, GimelSec, hake, hickuphh3, horsefacts, hubble, IllIllI, MaratCerby, MiloTruck, minhquanym, PPrieditis, reassor, shenwilly, smiling_heretic, TrungOre, VAD37, and WatchPug
Some ERC20 tokens charge a transaction fee for every transfer (used to encourage staking, add to liquidity pool, pay a fee to contract owner, etc.). If any such token is used in the createVault() function, either the token cannot be withdrawn from the contract (due to insufficient token balance), or it could be exploited by other such token holders and the Cally contract would lose economic value and some users would be unable to withdraw the underlying asset.
Plenty of ERC20 tokens charge a fee for every transfer (e.g. Safemoon and its forks), in which the amount of token received is less than the amount being sent. When a fee token is used as the token in the createVault() function, the amount received by the contract would be less than the amount being sent. To be more precise, the increase in the cally contract token balance would be less than vault.tokenIdOrAmount for such ERC20 token because of the fee.
vault.tokenType == TokenType.ERC721
? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
: ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
The implication is that both the exercise() function and the withdraw() function are guaranteed to revert if there's no other vault in the contract that contains the same fee tokens, due to insufficient token balance in the Cally contract.
When an attacker observes that a vault is being created that contains such fee tokens, the attacker could create a new vault himself that contains the same token, and then withdraw the same amount. Essentially the Cally contract would be paying the transfer fee for the attacker because of how the token amount is recorded. This causes loss of user fund and loss of value from the Cally contract. It would make economic sense for the attacker when the fee charged by the token accrue to the attacker. The attacker would essentially use the Cally contract as a conduit to generate fee income.
Recommend disallowing fee tokens from being used in the vault. This can be done by adding a require() statement to check that the amount increase of the token balance in the Cally contract is equal to the amount being sent by the caller of the createVault() function.
outdoteth (Cally) confirmed and commented:
reference issue: code-423n4/2022-05-cally-findings#39
HardlyDifficult (judge) decreased severity to Medium and commented:
This is a good description of the potential issue when a fee on transfer token is used.
Lowing to 2 (Medium). See code-423n4/org#3 for some discussion on how to consider the severity for these types of issues.
The attack described does leak value, but the vault could be recovered by transferring in the delta balance so that the contract has more than enough funds in order to exercise or withdraw. That plus these types of tokens are relatively rare is why I don't think this warrants a High severity.
Submitted by hickuphh3, also found by antonttc, berndartmueller, catchup, cccz, dipp, FSchmoede, GimelSec, hake, jah, jayjonah8, joestakey, kebabsec, Kenshin, Kumpa, MiloTruck, minhquanym, peritoflores, rfa, shenwilly, WatchPug, and ynnad
The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:
- OpenZeppelin’s documentation discourages the use of
transferFrom(), usesafeTransferFrom()whenever possible - Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the
onERC721Received()function, which is only triggered in thesafeTransferFrom()function and not intransferFrom()
Call the safeTransferFrom() method instead of transferFrom() for NFT transfers. Note that the CallyNft contract should inherit the ERC721TokenReceiver contract as a consequence.
abstract contract CallyNft is ERC721("Cally", "CALL"), ERC721TokenReceiver {...}outdoteth (Cally) confirmed and resolved:
the fix for this issue is here; outdoteth/cally#4
Submitted by GimelSec, also found by antonttc
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L158-L201
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L296
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L345
When calling createVault(), tokenType could be different from token’s type. If a user accidentally used the wrong tokenType, it could lead to two different results.
If token is an ERC20 token and the user uses TokenType.ERC721 as tokenType. It is less harmful, since ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount) still works when vault.token is actually ERC20 token.
However, if token is an ERC721 token and the user uses TokenType.ERC20 as tokenType. When doing creatVault(), ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount) works fine. But when doing exercise() or withdraw(), ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount); doesn’t work since ERC721 doesn’t implement safeTransfer() function. In consequence, the ERC721 token is frozen in the vault.
createVault() does not confirm whether tokenType and token’s type are the same.
But the token can still be transferred into this contract. Since transferFrom() is implemented in ERC20 and safeTransferFrom() is implemented in ERC721
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L158-L201
function createVault(
uint256 tokenIdOrAmount,
address token,
uint8 premiumIndex,
uint8 durationDays,
uint8 dutchAuctionStartingStrikeIndex,
uint256 dutchAuctionReserveStrike,
TokenType tokenType
) external returns (uint256 vaultId) {
require(premiumIndex < premiumOptions.length, "Invalid premium index");
require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
require(durationDays > 0, "durationDays too small");
require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");
Vault memory vault = Vault({
tokenIdOrAmount: tokenIdOrAmount,
token: token,
premiumIndex: premiumIndex,
durationDays: durationDays,
dutchAuctionStartingStrikeIndex: dutchAuctionStartingStrikeIndex,
currentExpiration: uint32(block.timestamp),
isExercised: false,
isWithdrawing: false,
tokenType: tokenType,
currentStrike: 0,
dutchAuctionReserveStrike: dutchAuctionReserveStrike
});
// vault index should always be odd
vaultIndex += 2;
vaultId = vaultIndex;
_vaults[vaultId] = vault;
// give msg.sender vault token
_mint(msg.sender, vaultId);
emit NewVault(vaultId, msg.sender, token);
// transfer the NFTs or ERC20s to the contract
vault.tokenType == TokenType.ERC721
? ERC721(vault.token).transferFrom(msg.sender, address(this), vault.tokenIdOrAmount)
: ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
}
However when doing exercise() or withdraw(), it always reverts since ERC721 doesn’t implement safeTransfer(). The ERC721 token is frozen in the contract.
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L296
function exercise(uint256 optionId) external payable {
…
// transfer the NFTs or ERC20s to the exerciser
vault.tokenType == TokenType.ERC721
? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
: ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
}
https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L345
function withdraw(uint256 vaultId) external nonReentrant {
…
// transfer the NFTs or ERC20s back to the owner
vault.tokenType == TokenType.ERC721
? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
: ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);
}
Confirm whether tokenType and token’s type are the same in createVault().
outdoteth (Cally) disputed, disagreed with severity and commented:
HardlyDifficult (judge) decreased severity to Medium and commented:
There were a lot of reports recommending a similar change, but this is one of the few that points our a critical issue that could arise in the current state.
Although the issue only occurs when the original vault creator makes a user error, the fact that their NFT becomes unrecoverable makes this a Medium Risk concern.
For this contest, 67 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by hubble received the top score from the judge.
The following wardens also submitted reports: VAD37, IllIllI, shung, MiloTruck, ellahi, MaratCerby, WatchPug, reassor, Picodes, shenwilly, 0xDjango, Bludya, pmerkleplant, hake, joestakey, robee, cccz, hyh, sseefried, hickuphh3, seanamani, djxploit, dirk_y, Kumpa, fatherOfBlocks, pedroais, catchup, radoslav11, csanuragjain, BowTiedWardens, Cityscape, sorrynotsorry, z3s, 0xsanson, minhquanym, sikorico, Czar102, jah, 0x4non, bobirichman, antonttc, BondiPestControl, 0x1f8b, FSchmoede, hansfriese, rfa, 0xf15ers, TrungOre, Aits, 242, Waze, horsefacts, Funen, Kenshin, mics, CertoraInc, Hawkeye, Ruhum, dipp, 0x1337, cryptphi, BouSalman, eccentricexit, AlleyCat, delfin454000, and JDeryl.
Function : createVault() in Cally.sol
line 169 require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
If the current error message is followed, user will never be able to successfully createVault()
Correct error message string
require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike more than Starting strike");
VaultId are odd in number; if a valultId of event number is given, the function valuts() will return misleading information.
Function valuts() in Cally.sol
Check if the valutID parameter is of vault type, by adding a require statement
function vaults(uint256 vaultId) external view returns (Vault memory) {
require(vaultId % 2 != 0, "Not vault type");
return _vaults[vaultId];
}
If the option is already exercised, the vault owner should not be allowed to call the initiateWithdraw() function.
Add a require statement in the function initiateWithdraw()
require(vault.isExercised == false, "Vault already exercised");
If a value of 0 is given for durationDays in the createVault() function, the transaction will revert with an ambigous message "durationDays too small" It can better stated as given below
Error message string can be changed as below.
line 170 require(durationDays > 0, "durationDays cannot be zero");
To reduce gas and storage, the protocol has currently designed to store the index of the premiumOptions[] & strikeOptions[] in the Vault structure.
This is too restrictive and may not be future proof.
One suggestion is to add another member unit8 premiumMultiplier (with default value of 1) in the Vault struct, and users can have combination of values of the premiumMultiplier and premiumIndex to define more range of premium values if required.
Same suggestion applies for adding a multiplier for the strikeOptions[]
When feeRate is changed at setFee(Cally.sol), no event is raised. It would be important to raise this event for any external integration with this system.
Contract : Cally.sol Function : setFee
event definition
event FeeRateUpdated(uint256 newFeeRate);
event emit at setFee function
require(feeRate_ != feeRate, "new feeRate should be different");
feeRate = feeRate_;
emit FeeRateUpdated(feeRate_);
When beneficiary is changed at setVaultBeneficiary(Cally.sol), no event is raised. It would be important to raise this event for any external integration with this system.
Contract : Cally.sol Function : setVaultBeneficiary
event definition
event VaultBeneficiaryUpdated(uint256 indexed vaultId, address indexed beneficiary);
event emit at setVaultBeneficiary function
emit VaultBeneficiaryUpdated(vaultId, beneficiary);
In Cally.sol, function buyOption the following is the order of lines. line 208 require(vaultId % 2 != 0, "Not vault type"); line 211 Vault memory vault = _vaults[vaultId];
This can be made consistent with other functions by changing the order.
Vault memory vault = _vaults[vaultId]; require(vaultId % 2 != 0, "Not vault type");
The value of vaultIndex = 1 is never assigned to any vault.
// vault index should always be odd
vaultIndex += 2;
vaultId = vaultIndex;
_vaults[vaultId] = vault;
This can be changed to as below, so that vaultID = 1 is also used
vaultId = vaultIndex;
// vault index should always be odd
vaultIndex += 2;
_vaults[vaultId] = vault;
high quality report
For this contest, 59 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: BowTiedWardens, shung, rotcivegaf, TomFrenchBlockchain, MaratCerby, MiloTruck, VAD37, hickuphh3, Bludya, WatchPug, robee, rfa, RagePit, defsec, TerrierLover, joestakey, 0x4non, CertoraInc, horsefacts, 0x1f8b, djxploit, Kenshin, gzeon, ellahi, minhquanym, sikorico, antonttc, 0xf15ers, Waze, fatherOfBlocks, pmerkleplant, 0xkatana, Cityscape, MadWookie, bobirichman, catchup, oyc_109, samruna, 0xDjango, _Adam, delfin454000, 0xNazgul, 0xsanson, Picodes, hansfriese, mics, simon135, DavidGialdi, ignacio, Tadashi, Funen, hake, z3s, 0v3rf10w, Hawkeye, Fitraldys, jonatascm, and FSchmoede.
| Issue | Instances | |
|---|---|---|
| 1 | Full vault details unnecessarily fetched twice in each call to buyOption() |
1 |
| 2 | Re-creation of short memory arrays is cheaper than fetching indecies from a storage array |
2 |
| 3 | Use existing stack cache variable rather than re-fetching state variable | 1 |
| 4 | Cheaper to split struct if only part of it is updated frequently | 1 |
| 5 | Store actual beneficiary rather than deciding every time | 2 |
| 6 | Use unchecked {} for calculations that cannot overflow |
1 |
| 7 | Also _burn() the vault to get a gas refund |
1 |
| 8 | State variables should be cached in stack variables rather than re-reading them from storage | 3 |
| 9 | <x> += <y> costs more gas than <x> = <x> + <y> for state variables |
2 |
| 10 | <array>.length should not be looked up in every loop of a for-loop |
1 |
| 11 | ++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loops |
1 |
| 12 | Offsets should only be calculated once per loop | 1 |
| 13 | Access mappings directly rather than using accessor functions | 5 |
| 14 | Cheaper input valdiations should come before expensive operations | 2 |
| 15 | Not using the named return variables when a function returns, wastes deployment gas | 2 |
| 16 | Use a more recent version of solidity | 1 |
| 17 | Using > 0 costs more gas than != 0 when used on a uint in a require() statement |
1 |
| 18 | It costs more gas to initialize variables to zero than to let the default of zero be applied | 2 |
| 19 | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) |
1 |
| 20 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead |
10 |
| 21 | Using private rather than public for constants, saves gas |
1 |
| 22 | Don't compare boolean expressions to boolean literals | 3 |
| 23 | Duplicated require()/revert() checks should be refactored to a modifier or function |
4 |
| 24 | Multiplication/division by two should use bit shifting | 3 |
| 25 | Use custom errors rather than revert()/require() strings to save deployment gas |
32 |
| 26 | Functions guaranteed to revert when called by normal users can be marked payable |
2 |
Total: 86 instances over 26 issues
getPremium() should be changed to have its argument be an index into the array, rather than looking up the vault again. Since this is in one of the two frequently-called functions, it'll save a lot of gas
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
223: uint256 premium = getPremium(vaultId);Since the arrays are relatively short, it's cheaper to have a pure virtual function that re-creates them every time just to fetch a specific index, rather than incuring a Gcoldsload (2100 gas). Since this is in one of the two frequently-called functions, it'll save a lot of gas
There are 2 instances of this issue:
File: ./contracts/src/Cally.sol #1
90: uint256[] public premiumOptions = [0.01 ether, 0.025 ether, 0.05 ether, 0.075 ether, 0.1 ether, 0.25 ether, 0.5 ether, 0.75 ether, 1.0 ether, 2.5 ether, 5.0 ether, 7.5 ether, 10 ether, 25 ether, 50 ether, 75 ether, 100 ether];File: contracts/src/Cally.sol #2
92: uint256[] public strikeOptions = [1 ether, 2 ether, 3 ether, 5 ether, 8 ether, 13 ether, 21 ether, 34 ether, 55 ether, 89 ether, 144 ether, 233 ether, 377 ether, 610 ether, 987 ether, 1597 ether, 2584 ether, 4181 ether, 6765 ether];Using auctionStartTimestamp defined above will save a Gwarmaccess (100 gas). Since this is in one of the two frequently-called functions, it'll save a lot of gas
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
233: vault.currentExpiration + AUCTION_DURATION,The currentStrike and currentExpiration fields are updated frequently so they should be in a separate struct rather than re-writing the whole struct every time
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
230 // set new currentStrike
231 vault.currentStrike = getDutchAuctionStrike(
232 strikeOptions[vault.dutchAuctionStartingStrikeIndex],
233 vault.currentExpiration + AUCTION_DURATION,
234 vault.dutchAuctionReserveStrike
235 );
236
237 // set new expiration
238 vault.currentExpiration = uint32(block.timestamp) + (vault.durationDays * 1 days);
239
240 // update the vault with the new option expiration and strike
241: _vaults[vaultId] = vault;If the beneficiary array assigns the owner every time the NFT is minted or transferred, the mapping can be used directly, saving the gas overhead of function calls
There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
249: address beneficiary = getVaultBeneficiary(vaultId);File: contracts/src/Cally.sol #2
289: ethBalance[getVaultBeneficiary(vaultId)] += msg.value - fee;The subtraction below can be unchecked {} because of the check that comes before it
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
417: uint256 delta = auctionEndTimestamp > block.timestamp ? auctionEndTimestamp - block.timestamp : 0;There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
275: _burn(optionId);The instances below point to the second+ access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 3 instances of this issue:
File: contracts/src/Cally.sol #1
/// @audit strikeOptions
169: require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");File: contracts/src/Cally.sol #2
/// @audit feeRate
284: fee = (msg.value * feeRate) / 1e18;File: contracts/src/Cally.sol #3
/// @audit vaultIndex
189: vaultId = vaultIndex;There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
188: vaultIndex += 2;File: contracts/src/Cally.sol #2
285: protocolUnclaimedFees += fee;The overheads outlined below are PER LOOP, excluding the first loop
- storage arrays incur a Gwarmaccess (100 gas)
- memory arrays use
MLOAD(3 gas) - calldata arrays use
CALLDATALOAD(3 gas)
Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There is 1 instance of this issue:
File: contracts/src/CallyNft.sol #1
244: for (uint256 i = 0; i < data.length; i++) {[G-11] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
There is 1 instance of this issue:
File: contracts/src/CallyNft.sol #1
244: for (uint256 i = 0; i < data.length; i++) {i * 2 is calculated twice per loop, wasting gas
There is 1 instance of this issue:
File: contracts/src/CallyNft.sol #1
244 for (uint256 i = 0; i < data.length; i++) {
245 str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))];
246 str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))];
247: }Saves having to do two JUMP instructions, along with stack setup
There are 5 instances of this issue:
File: contracts/src/Cally.sol
214: require(ownerOf(vaultId) != address(0), "Vault does not exist");
263: require(msg.sender == ownerOf(optionId), "You are not the owner");
307: require(msg.sender == ownerOf(vaultId), "You are not the owner");
323: require(msg.sender == ownerOf(vaultId), "You are not the owner");
354: require(msg.sender == ownerOf(vaultId), "Not owner");There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
211: require(vaultId % 2 != 0, "Not vault type");File: contracts/src/Cally.sol #2
214: require(ownerOf(vaultId) != address(0), "Vault does not exist");There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
382: return currentBeneficiary == address(0) ? ownerOf(vaultId) : currentBeneficiary;File: contracts/src/Cally.sol #2
396: return premiumOptions[vault.premiumIndex];Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There is 1 instance of this issue:
File: contracts/lib/base64/base64.sol #1
3: pragma solidity >=0.6.0;This change saves 6 gas per instance
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
170: require(durationDays > 0, "durationDays too small");There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
282: uint256 fee = 0;File: contracts/src/CallyNft.sol #2
244: for (uint256 i = 0; i < data.length; i++) {Saves 6 gas PER LOOP
There is 1 instance of this issue:
File: contracts/src/CallyNft.sol #1
244: for (uint256 i = 0; i < data.length; i++) {When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
There are 10 instances of this issue:
File: contracts/src/Cally.sol
76: uint8 premiumIndex; // indexes into `premiumOptions`
77: uint8 durationDays; // days
78: uint8 dutchAuctionStartingStrikeIndex; // indexes into `strikeOptions`
79: uint32 currentExpiration;
87: uint32 public constant AUCTION_DURATION = 24 hours;
161: uint8 premiumIndex,
162: uint8 durationDays,
163: uint8 dutchAuctionStartingStrikeIndex,
227: uint32 auctionStartTimestamp = vault.currentExpiration;
408: uint32 auctionEndTimestamp,If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: contracts/src/Cally.sol #1
87: uint32 public constant AUCTION_DURATION = 24 hours;if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There are 3 instances of this issue:
File: contracts/src/Cally.sol #1
217: require(vault.isExercised == false, "Vault already exercised");File: contracts/src/Cally.sol #2
220: require(vault.isWithdrawing == false, "Vault is being withdrawn");File: contracts/src/Cally.sol #3
328: require(vault.isExercised == false, "Vault already exercised");Saves deployment costs
There are 4 instances of this issue:
File: contracts/src/Cally.sol #1
304: require(vaultId % 2 != 0, "Not vault type");File: contracts/src/Cally.sol #2
328: require(vault.isExercised == false, "Vault already exercised");File: contracts/src/Cally.sol #3
323: require(msg.sender == ownerOf(vaultId), "You are not the owner");File: contracts/src/CallyNft.sol #4
42: require(to != address(0), "INVALID_RECIPIENT");<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas
There are 3 instances of this issue:
File: contracts/src/CallyNft.sol #1
241: bytes memory str = new bytes(2 + data.length * 2);File: contracts/src/CallyNft.sol #2
245: str[2 + i * 2] = alphabet[uint256(uint8(data[i] >> 4))];File: contracts/src/CallyNft.sol #3
246: str[3 + i * 2] = alphabet[uint256(uint8(data[i] & 0x0f))];Custom errors are available from solidity version 0.8.4. The instances below match or exceed that version
There are 32 instances of this issue:
File: contracts/src/Cally.sol
167: require(premiumIndex < premiumOptions.length, "Invalid premium index");
168: require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
169: require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
170: require(durationDays > 0, "durationDays too small");
171: require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");
211: require(vaultId % 2 != 0, "Not vault type");
214: require(ownerOf(vaultId) != address(0), "Vault does not exist");
217: require(vault.isExercised == false, "Vault already exercised");
220: require(vault.isWithdrawing == false, "Vault is being withdrawn");
224: require(msg.value >= premium, "Incorrect ETH amount sent");
228: require(block.timestamp >= auctionStartTimestamp, "Auction not started");
260: require(optionId % 2 == 0, "Not option type");
263: require(msg.sender == ownerOf(optionId), "You are not the owner");
269: require(block.timestamp < vault.currentExpiration, "Option has expired");
272: require(msg.value == vault.currentStrike, "Incorrect ETH sent for strike");
304: require(vaultId % 2 != 0, "Not vault type");
307: require(msg.sender == ownerOf(vaultId), "You are not the owner");
320: require(vaultId % 2 != 0, "Not vault type");
323: require(msg.sender == ownerOf(vaultId), "You are not the owner");
328: require(vault.isExercised == false, "Vault already exercised");
329: require(vault.isWithdrawing, "Vault not in withdrawable state");
330: require(block.timestamp > vault.currentExpiration, "Option still active");
353: require(vaultId % 2 != 0, "Not vault type");
354: require(msg.sender == ownerOf(vaultId), "Not owner");
436: require(from == _ownerOf[id], "WRONG_FROM");
437: require(to != address(0), "INVALID_RECIPIENT");
438 require(
439 msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
440 "NOT_AUTHORIZED"
441: );
456: require(_ownerOf[tokenId] != address(0), "URI query for NOT_MINTED token");File: contracts/src/CallyNft.sol
15: require(to != address(0), "INVALID_RECIPIENT");
16: require(_ownerOf[id] == address(0), "ALREADY_MINTED");
36: require(owner != address(0), "ZERO_ADDRESS");
42: require(to != address(0), "INVALID_RECIPIENT");If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 2 instances of this issue:
File: contracts/src/Cally.sol #1
119: function setFee(uint256 feeRate_) external onlyOwner {File: contracts/src/Cally.sol #2
124: function withdrawProtocolFees() external onlyOwner returns (uint256 amount) {this is the best gas report imo - hats off to you!
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.