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

Too much fee charged when Seaport is partially filled #71

Open
code423n4 opened this issue Nov 11, 2022 · 3 comments
Open

Too much fee charged when Seaport is partially filled #71

code423n4 opened this issue Nov 11, 2022 · 3 comments
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 M-02 satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report 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-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L136-L147

Vulnerability details

Impact

When a user fulfills an order using SeaportProxy, fees are charged in the _handleFees function based on orders.price.

    function _handleFees(
        BasicOrder[] calldata orders,
        uint256 feeBp,
        address feeRecipient
    ) private {
        address lastOrderCurrency;
        uint256 fee;
        uint256 ordersLength = orders.length;

        for (uint256 i; i < ordersLength; ) {
            address currency = orders[i].currency;
            uint256 orderFee = (orders[i].price * feeBp) / 10000;

According to the Seaport documentation, Seaport allows partial fulfillment of orders, which results in too much fee being charged when an order is partially filled
https://docs.opensea.io/v2.0/reference/seaport-overview#partial-fills

Consider feeBp == 2%
The order on Seaport has a fill status of 0/100 and each item is worth 1 eth.
User A fulfills the order using LooksRareAggregator.execute and sends 102 ETH, where order.price == 100 ETH.
Since the other user fulfilled the order before User A, when User A fulfills the order, the order status is 99/100
Eventually User A buys an item for 1 ETH but pays a fee of 2 ETH.

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/proxies/SeaportProxy.sol#L136-L147

Tools Used

None

Recommended Mitigation Steps

Consider charging fees based on the user's actual filled price

@code423n4 code423n4 added 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 labels Nov 11, 2022
code423n4 added a commit that referenced this issue Nov 11, 2022
@c4-sponsor
Copy link

0xhiroshi marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Nov 24, 2022
@0xhiroshi
Copy link

We are dropping fees completely.

@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 11, 2022
@C4-Staff C4-Staff added M-02 selected for report This submission will be included/highlighted in the audit report labels Dec 20, 2022
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 M-02 satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants