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 #307

Open
code423n4 opened this issue May 14, 2022 · 1 comment
Open

QA Report #307

code423n4 opened this issue May 14, 2022 · 1 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

Comments

@code423n4
Copy link
Contributor

(Low) feeRate can be modified for existing vaults

feeRate is a parameter that controls the fee applied on exercise. It can be set by the function:

function setFee(uint256 feeRate_) external onlyOwner {
    feeRate = feeRate_; 
} 

So it can be changed by the owner at any time, changing also the fee payed by existing vaults.

Proof of concept

Alice is a trader looking for a delta neutral position on her NFTs. She opens a vault with strike 10 ETH and fee 0.5%. She's happy getting 9.95 ETH if the option is exercised. During the call lifetime the fee gets increased up to 1%. Now Alice will get 9.90 ETH, an amount which may bring her EV negative.

Recommendations

It's suggested to save feeRate into a vault struct during createVault or buyOption. This value can then be used during exercise instead of the global variable.

(Low) Mistake in tokenURI encoding

In the tokenURI string calculation, we need to pass the vault premium. At line L464 there's a getPremium(vault.premiumIndex) but the function actually wants a vaultId:

    /// @notice Get the fixed option premium for a vault
    /// @param vaultId The tokenId of the vault to fetch the premium for
    /// @return premium The premium for the vault
    function getPremium(uint256 vaultId) public view returns (uint256 premium) {
        Vault memory vault = _vaults[vaultId];
        return premiumOptions[vault.premiumIndex];
    }

As a consequence the premium gets mispriced on UI, leading to unpredictable errors.

Recommendations

Change L464 to premiumOptions[vault.premiumIndex].

(Info) Wrong math terminology: strike curve is not "exponential"

Documentation (L400) claims that during dutch auction, strike decreases in time "exponentially". This term is correct colloquially, but it's technically misused since the curve is actually a parabola instead of an exponential.

Suggested removing the adverb or finding a more appropriate one.

(Info) Missing event when changing feeRate

The function setFee doesn't emit an event when changing feeRate.
It's generally suggested to add events for every important parameter that can get changed, for easier off-chain monitoring.

@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 14, 2022
code423n4 added a commit that referenced this issue May 14, 2022
@outdoteth
Copy link
Collaborator

this can be updated to be medium severity:
(Low) feeRate can be modified for existing vaults; #47

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

2 participants