The _matcho() is not implemented properly

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/511636d889742296a54392875a35e4c0c4727bb7/contracts/RubiconMarket.sol#L1313

Vulnerability details

Impact

The _matcho() function is not implemented properly, it might revert while the taking amount is greater than the best offer's amount, leads to order matching failure.

Proof of Concept

The following test script shows how to trigger the issue.

pragma solidity ^0.8.0;

import "../../contracts/compound-v2-fork/WhitePaperInterestRateModel.sol";
import "../../contracts/compound-v2-fork/ComptrollerInterface.sol";
import "../../contracts/compound-v2-fork/CErc20Delegator.sol";
import "../../contracts/compound-v2-fork/CErc20Delegate.sol";
import "../../contracts/compound-v2-fork/Comptroller.sol";
import "../../contracts/utilities/MarketAidFactory.sol";
import "../../contracts/periphery/TokenWithFaucet.sol";
import "../../contracts/utilities/MarketAid.sol";
import "../../contracts/periphery/WETH9.sol";
import "../../contracts/RubiconMarket.sol";
import "forge-std/Test.sol";

/// @notice proxy isn't used here
contract MatchingOfferNotWorking is Test {
  //========================CONSTANTS========================
  address public owner;
  address FEE_TO = 0x0000000000000000000000000000000000000FEE;
  address taker = address(0x1234);
  address maker = address(0x5678);
  // core contracts
  RubiconMarket market;
  // test tokens
  TokenWithFaucet RUBI;
  TokenWithFaucet USDC;

  // deployRubiconProtocolFixture()
  function setUp() public {
    owner = msg.sender;

    // deploy new Market instance and init
    market = new RubiconMarket();
    market.initialize(FEE_TO);
    market.setFeeBPS(10);

    // deploy test tokens
    RUBI = new TokenWithFaucet(address(this), "RUBI", "RUBI", 18);
    USDC = new TokenWithFaucet(address(this), "Test Stablecoin", "USDC", 6);

    // add some $$$ to victim and attacker
    RUBI.faucet();
    USDC.faucet();
    RUBI.transfer(maker, 1000e18);
    USDC.transfer(taker, 10000e6);

    vm.startPrank(taker);
    USDC.approve(address(market), type(uint256).max);
    vm.stopPrank();

    vm.startPrank(maker);
    RUBI.approve(address(market), type(uint256).max);
    vm.stopPrank();
  }

  //========================MARKET_TESTS========================

  function test_MatchingOfferNotWorking() public {
   // 1. maker submits an offer for selling 1 RUBI at 100 USDC/RUBI
    uint256 makerRubiBalanceBefore = RUBI.balanceOf(maker);
    vm.startPrank(maker);
    uint256 id = market.offer(1e18, RUBI, 100e6, USDC); 
    vm.stopPrank();
    assertGt(id, 0);
    uint256 makerRubiBalanceAfter = RUBI.balanceOf(maker);
    assertEq(makerRubiBalanceBefore - makerRubiBalanceAfter, 1e18);
    
    // 2. now the taker submits another offer for buying 10 RUBI at 100 USDC/RUBI
    //    we expect the above selling offer to be fulfilled successfully.
    //    Meanwhile, a new 9 RUBI buying offer should be created,
    //    but actually the this offer will be reverted.
    vm.startPrank(taker);
    // buying RUBI/USDC at 100 USDC/RUBI
    vm.expectRevert();
    id = market.offer(1000e6, USDC, 10e18, RUBI); 
    vm.stopPrank();
  }

}

And the log details

Running 1 test for test/foundry-tests/MatchingOfferNotWorking.t.sol:MatchingOfferNotWorking
[PASS] test_MatchingOfferNotWorking() (gas: 422566)
Traces:
  [422566] MatchingOfferNotWorking::test_MatchingOfferNotWorking()
    ├─ [2629] TokenWithFaucet::balanceOf(0x0000000000000000000000000000000000005678) [staticcall]
    │   └─ ← 1000000000000000000000
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000005678)
    │   └─ ← ()
    ├─ [246628] RubiconMarket::offer(1000000000000000000, TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 100000000, TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
    │   ├─ [30321] TokenWithFaucet::transferFrom(0x0000000000000000000000000000000000005678, RubiconMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000000000000000000)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000005678, to: RubiconMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1000000000000000000)
    │   │   └─ ← true
    │   ├─ emit LogItemUpdate(id: 1)
    │   ├─ emit emitOffer(id: 0x0000000000000000000000000000000000000000000000000000000000000001, pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148, maker: 0x0000000000000000000000000000000000005678, pay_gem: TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], buy_gem: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], pay_amt: 1000000000000000000, buy_amt: 100000000)
    │   ├─ emit LogSortedOffer(id: 1)
    │   └─ ← 1
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [629] TokenWithFaucet::balanceOf(0x0000000000000000000000000000000000005678) [staticcall]
    │   └─ ← 999000000000000000000
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001234)
    │   └─ ← ()
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [167743] RubiconMarket::offer(1000000000, TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 10000000000000000000, TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   ├─ [32321] TokenWithFaucet::transferFrom(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000000FEE, 9999)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001234, to: 0x0000000000000000000000000000000000000FEE, value: 9999)
    │   │   └─ ← true
    │   ├─ [25521] TokenWithFaucet::transferFrom(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000005678, 99990000)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001234, to: 0x0000000000000000000000000000000000005678, value: 99990000)
    │   │   └─ ← true
    │   ├─ [24995] TokenWithFaucet::transfer(0x0000000000000000000000000000000000001234, 999900000000000000)
    │   │   ├─ emit Transfer(from: RubiconMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: 0x0000000000000000000000000000000000001234, value: 999900000000000000)
    │   │   └─ ← true
    │   ├─ emit LogItemUpdate(id: 1)
    │   ├─ emit emitTake(id: 0x0000000000000000000000000000000000000000000000000000000000000001, pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148, taker: 0x0000000000000000000000000000000000001234, maker: 0x0000000000000000000000000000000000005678, pay_gem: TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], buy_gem: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], take_amt: 999900000000000000, give_amt: 99990000)
    │   ├─ emit emitFee(id: 0x0000000000000000000000000000000000000000000000000000000000000001, taker: 0x0000000000000000000000000000000000001234, feeTo: 0x0000000000000000000000000000000000000FEE, pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148, asset: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], feeAmt: 9999)
    │   ├─ emit LogMatch(id: 0, amount: 1000000000000000000)
    │   ├─ [3621] TokenWithFaucet::transferFrom(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000000FEE, 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001234, to: 0x0000000000000000000000000000000000000FEE, value: 0)
    │   │   └─ ← true
    │   ├─ [3621] TokenWithFaucet::transferFrom(0x0000000000000000000000000000000000001234, 0x0000000000000000000000000000000000005678, 9999)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001234, to: 0x0000000000000000000000000000000000005678, value: 9999)
    │   │   └─ ← true
    │   ├─ [3095] TokenWithFaucet::transfer(0x0000000000000000000000000000000000001234, 99990000000000)
    │   │   ├─ emit Transfer(from: RubiconMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: 0x0000000000000000000000000000000000001234, value: 99990000000000)
    │   │   └─ ← true
    │   ├─ emit LogItemUpdate(id: 1)
    │   ├─ emit emitTake(id: 0x0000000000000000000000000000000000000000000000000000000000000001, pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148, taker: 0x0000000000000000000000000000000000001234, maker: 0x0000000000000000000000000000000000005678, pay_gem: TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], buy_gem: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], take_amt: 99990000000000, give_amt: 9999)
    │   ├─ emit emitFee(id: 0x0000000000000000000000000000000000000000000000000000000000000001, taker: 0x0000000000000000000000000000000000001234, feeTo: 0x0000000000000000000000000000000000000FEE, pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148, asset: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], feeAmt: 0)
    │   ├─ emit LogMatch(id: 0, amount: 100000000000000)
    │   └─ ← "EvmError: Revert"
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 2.67ms

Let's take a look at this line, we find the actual take_amt is 0.9999e18 RUBI rather than the expected 1e18, the difference is exact the 0.01% of fee.

emit emitTake(id: 0x0000000000000000000000000000000000000000000000000000000000000001,
 pair: 0x466a72696a5097346305fd11b91586c4ba7becfaf99e93d36de04eca0fe58148,
 taker: 0x0000000000000000000000000000000000001234,
 maker: 0x0000000000000000000000000000000000005678,
 pay_gem: TokenWithFaucet: [0x2e234DAe75C793f67A35089C9d99245E1C58470b],
 buy_gem: TokenWithFaucet: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a],
 take_amt: 999900000000000000, 
give_amt: 99990000)

Back to source code, we can see the issue arises on L1313 of _matcho(), where m_pay_amt should be some thing like m_pay_amt + fee.

File: contracts\RubiconMarket.sol
1273:     function _matcho(
1274:         uint256 t_pay_amt, //taker sell how much
1275:         ERC20 t_pay_gem, //taker sell which token
1276:         uint256 t_buy_amt, //taker buy how much
1277:         ERC20 t_buy_gem, //taker buy which token
1278:         uint256 pos, //position id
1279:         bool rounding, //match "close enough" orders?
1280:         address owner,
1281:         address recipient
1282:     ) internal returns (uint256 id) {
1283:         uint256 best_maker_id; //highest maker id
1284:         uint256 t_buy_amt_old; //taker buy how much saved
1285:         uint256 m_buy_amt; //maker offer wants to buy this much token
1286:         uint256 m_pay_amt; //maker offer wants to sell this much token
1287: 
1288:         // there is at least one offer stored for token pair
1289:         while (_best[address(t_buy_gem)][address(t_pay_gem)] > 0) {
1290:             best_maker_id = _best[address(t_buy_gem)][address(t_pay_gem)];
1291:             m_buy_amt = offers[best_maker_id].buy_amt;
1292:             m_pay_amt = offers[best_maker_id].pay_amt;
1293: 
...
1313:             buy(best_maker_id, min(m_pay_amt, t_buy_amt));
1314:             emit LogMatch(id, min(m_pay_amt, t_buy_amt));
1315:             t_buy_amt_old = t_buy_amt;
1316:             t_buy_amt = sub(t_buy_amt, min(m_pay_amt, t_buy_amt));
1317:             t_pay_amt = mul(t_buy_amt, t_pay_amt) / t_buy_amt_old;
1318: 
1319:             if (t_pay_amt == 0 || t_buy_amt == 0) {
1320:                 break;
1321:             }
1322:         }
1323: 
...
1341:     }

Tools Used

Manually review

Recommended Mitigation Steps

See PoC

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1336

[c4-judge]

HickupHH3 marked the issue as selected for report