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

Open
code423n4 opened this issue May 14, 2022 · 3 comments
Open

QA Report #294

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

[L-01] Owner can frontrun exercise to increase fees

A malicious owner account can observe and frontrun calls to exercise and extract 100% of the strike price as a protocol fee.

Scenario:

  1. A malicious owner observes a call to exercise in the mempool.
  2. The owner frontruns exercise and calls setFee to set feeRate to 100%
  3. The full strike price is paid as a protocol fee, and 0 ETH are credited to the vault beneficiary.

Recommendation: Ensure the contract owner is a timelock proxy with a waiting period for parameter changes. Emit an event on changes to feeRate (See N-01 below).

[L-02] Beneficiary is credited additional ETH above premium

The Cally#buyOption function ensures that the caller sends an ETH amount equal to or greater than the calculated premium:

buyOption#L224

       require(msg.value >= premium, "Incorrect ETH amount sent");

It then credits the beneficiary with an amount equal to msg.value:

buyOption#L250

        ethBalance[beneficiary] += msg.value;

If the caller of buyOption sends excess ETH above the premium amount, this additional amount is credited to the beneficiary.

Recommendation: If this is intentional, clearly document this behavior for end users. If not, consider requiring an exact premium amount rather than accepting additional ETH.

QA

[N-01] Emit events for permissioned parameter changes

The permissioned function setFee updates the feeRate parameter, but does not emit an event. Consider emitting a SetFee event that logs the previous and new feeRate values. This enables you to monitor off chain for suspicious activity, and allows end users to observe and trust permissioned changes to this parameter.

Cally.sol#setFee

    /// @notice Sets the fee that is applied on exercise
    /// @param feeRate_ The new fee rate: fee = 1% = (1 / 100) * 1e18
    function setFee(uint256 feeRate_) external onlyOwner {
        feeRate = feeRate_;
    }
@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

these can be bumped to medium severity:
[L-01] Owner can frontrun exercise to increase fees: #47
[L-02] Beneficiary is credited additional ETH above premium: #84

@HardlyDifficult
Copy link
Collaborator

Moved L-01 to #323

@HardlyDifficult
Copy link
Collaborator

Moved L-02 to #324

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

3 participants