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

User's may accidentally overpay in buyOption() and the excess will be paid to the vault creator #84

Open
code423n4 opened this issue May 12, 2022 · 3 comments · Fixed by outdoteth/cally#9
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L223-L224

Vulnerability details

Impact

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.

Proof of Concept

buyOption() allows msg.value > premium

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

Recommended Mitigation Steps

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
Copy link
Collaborator

this issue is fixed in: outdoteth/cally#9

@outdoteth outdoteth reopened this May 17, 2022
@outdoteth outdoteth added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label May 17, 2022
@HardlyDifficult
Copy link
Collaborator

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.

@HickupHH3
Copy link

HickupHH3 commented Jun 8, 2022

QA report #182 should have its issue bumped up and marked as a duplicate IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants