Fee inclusivity calculations are inaccurate in RubiconMarket

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L578-L589

Vulnerability details

Impact

Trading in the RubiconMarket has associated fee costs that are paid by the taker of the offer. These fees include the protocol fee and a new "maker fee" introduced in v2. Fees are pulled from the taker (caller of the function) independently of the trade amount, which means fees are not included in the trade amount. These are implemented in the buy function of the base contract SimpleMarket:

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L337-L373

337:         /// @dev Fee logic added on taker trades
338:         uint256 fee = mul(spend, feeBPS) / 100_000;
339:         require(
340:             _offer.buy_gem.transferFrom(msg.sender, feeTo, fee),
341:             "Insufficient funds to cover fee"
342:         );
343: 
344:         // taker pay maker 0_0
345:         if (makerFee() > 0) {
346:             uint256 mFee = mul(spend, makerFee()) / 100_000;
347: 
348:             /// @dev Handle the v1 -> v2 migration case where if owner == address(0) we transfer this fee to _offer.recipient
349:             if (_offer.owner == address(0) && getRecipient(id) != address(0)) {
350:                 require(
351:                     _offer.buy_gem.transferFrom(
352:                         msg.sender,
353:                         _offer.recipient,
354:                         mFee
355:                     ),
356:                     "Insufficient funds to cover fee"
357:                 );
358:             } else {
359:                 require(
360:                     _offer.buy_gem.transferFrom(msg.sender, _offer.owner, mFee),
361:                     "Insufficient funds to cover fee"
362:                 );
363:             }
364: 
365:             emit emitFee(
366:                 bytes32(id),
367:                 msg.sender,
368:                 _offer.owner,
369:                 keccak256(abi.encodePacked(_offer.pay_gem, _offer.buy_gem)),
370:                 _offer.buy_gem,
371:                 mFee
372:             );
373:         }

One of the new additions in the RubiconMarket v2 is fee inclusivity, a feature that would allow users to operate on the market by including the fee directly in the specified amount. This is present in different places of the contract, but the core implementation can be founded in the calcAmountAfterFee function:

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L578-L589

578:     function calcAmountAfterFee(
579:         uint256 amount
580:     ) public view returns (uint256 _amount) {
581:         require(amount > 0);
582:         _amount = amount;
583:         _amount -= mul(amount, feeBPS) / 100_000;
584: 
585:         if (makerFee() > 0) {
586:             _amount -= mul(amount, makerFee()) / 100_000;
587:         }
588:     }

As we can see in the previous snippet, the function calculates the protocol fee and the marker fee based on the given amount, and substacts those values from the amount. This is an inaccurate calculation, as these fees later on will be calculated using this new value, which won't end up totalling the requested original amount. As an example, let's consider the case of 100 tokens, a 10% protocol fee and a 5% maker fee:

1. Initial amount is 1000 tokens.
2. Protocol fee is 1000 * 10% = 100 tokens.
3. Maker fee is 1000 * 5% = 50 tokens.
4. Resulting amount of calcAmountAfterFee will be 1000 - 100 - 50 = 850 tokens.
5. In the core buy function, the trade amount will be 850 tokens, and the function will then calculate fees based on this amount.
6. Protocol fee will be calculated as 850 * 10% = 85 and maker fee will be calculated as 850 * 5% = 42 tokens.
7. This means that the user will end up paying 850 for the trade, 85 for the protocol fee and 42 for the maker fee. 850 + 85 + 42 = 977 which is a bit less than the original 1000 tokens.

Proof of Concept

In the following test, Alice makes an offer to sell 30k USDC for 1 BTC. Bob will execute the trade to buy the complete 30k USDC with BTC. However, as the fee calculation is inaccurate, the trade will be executed for an amount less than expected and Bob will be left with some unspent BTC (0.000225 BTC).

Logs:
  Bob BTC balance: 225000000000000

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_RubiconMarket_buy_IncorrectFeeInclusivity() public {
    // Alice has 30k USDC, bob has 1 BTC
    USDC.mint(alice, 30_000 ether);
    BTC.mint(bob, 1 ether);

    // Alice creates the offer to sell USDC for BTC
    vm.prank(alice);
    USDC.approve(address(market), type(uint256).max);
    vm.prank(alice);
    bytes32 id = market.make(
        USDC, // pay
        BTC, // buy
        30_000 ether, // pay_amount
        1 ether // buy_amount
    );

    // Bob trades his BTC for USDC
    vm.prank(bob);
    BTC.approve(address(market), type(uint256).max);
    vm.prank(bob);
    market.buy(uint256(id), 30_000 ether);

    console.log("Bob BTC balance:", BTC.balanceOf(bob));
}

Recommendation

The correct calculation for the fee inclusivity amount should be as follows:

result = amount / (100% + protocol fee + maker fee)

For the example given in the previous section, this would result in an amount of 1000 / (100% + 10% + 5%) = 869.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

daoio marked the issue as sponsor confirmed

[c4-judge]

HickupHH3 marked issue #899 as primary and marked this issue as a duplicate of 899

[c4-judge]

HickupHH3 marked the issue as selected for report