Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #150

Open
code423n4 opened this issue May 13, 2022 · 0 comments
Open

QA Report #150

code423n4 opened this issue May 13, 2022 · 0 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Low risk findings:

1. tokenURI pass in the wrong parameter for getPremium

It should accept a vaultId, but here the code pass in premiumId

getPremium(vault.premiumIndex),

This will make the NFT showing the wrong trait.
P.S. I suggested changing the input parameter of getPremium from vaultId to premiumIndex, as described below (gas optimization #2)

2. wrong revert message in createVault

This revert message:

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

should instead be reserve strike too high.

Gas optimizations

1. pack variables feeRate and protocolUnclaimedFees together to save gas on exercise.

Because these 2 variables are used together in exercise function, (reading feeRate and updating protocolUnclaimedFees), packing them into the same storage slot will save you one more SLOAD if feeRate > 0. on average this saves around 2000 gas. (49k -> 47K)

2. change getPremium input to premiumIndex.

As it currently is, getPremium function receive vaultId read the vault from storage again and than get the premiumIndex. This will cause an extra SLOAD to load the vault from storage.
This can be avoided if we just pass in premiumIndex, as all 2 functions that call this already have their vault instance in memory.
This makes average cost of buyOption drops from 75.7K -> 74.8.

3. pack currentStrike and dutchAuctionReserveStrike into a single storage slot.

Since packing variables is highly used in the vault struct, i suggest also pack dutchAuctionReserveStrike and currentStrike to uint128 to save 1 SLOAD and SSTORE from read and write.
The gas saving from running the benchmark is 74.8K -> 73.5K

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 13, 2022
code423n4 added a commit that referenced this issue May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

1 participant