Anyone can cancel and get a refund of orders placed through the RubiconRouter

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L440-L452

Vulnerability details

Impact

Users will lose funds associated with orders they've placed, to other users that cancel the order

Proof of Concept

The RubiconRouter does no validation itself of whether a user should be able to cancel an order, and instead relies on RubiconMarket to do the checks:

File: contracts/RubiconRouter.sol   #1

440       function cancelForETH(uint256 id) external returns (bool outcome) {
441           (uint256 pay_amt, ERC20 pay_gem, , ) = RubiconMarket(
442               RubiconMarketAddress
443           ).getOffer(id);
444           require(
445               address(pay_gem) == wethAddress,
446               "trying to cancel a non WETH order"
447           );
448           // Cancel order and receive WETH here in amount of pay_amt
449           outcome = RubiconMarket(RubiconMarketAddress).cancel(id);
450           WETH9(wethAddress).withdraw(pay_amt);
451           msg.sender.transfer(pay_amt);
452       }

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L440-L452

The RubiconMarket only verifies that the message sender is the owner:

File: contracts/RubiconMarket.sol   #2

215       modifier can_cancel(uint256 id) virtual {
216           require(isActive(id));
217           require(getOwner(id) == msg.sender);
218           _;
219       }

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L215-L219

which will always be the RubiconRouter for orders placed through the RubiconRouter:

File: contracts/RubiconMarket.sol   #3

391       /// @notice Key function to make a new offer. Takes funds from the caller into market escrow.
392       function offer(
393           uint256 pay_amt,
394           ERC20 pay_gem,
395           uint256 buy_amt,
396           ERC20 buy_gem
397       ) public virtual can_offer synchronized returns (uint256 id) {
398           require(uint128(pay_amt) == pay_amt);
399           require(uint128(buy_amt) == buy_amt);
400           require(pay_amt > 0);
401           require(pay_gem != ERC20(0x0));
402           require(buy_amt > 0);
403           require(buy_gem != ERC20(0x0));
404           require(pay_gem != buy_gem);
405   
406           OfferInfo memory info;
407           info.pay_amt = pay_amt;
408           info.pay_gem = pay_gem;
409           info.buy_amt = buy_amt;
410           info.buy_gem = buy_gem;
411           info.owner = msg.sender;

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L391-L411

Tools Used

Code inspection

Recommended Mitigation Steps

Maintain a mapping from submitters to order IDs, inside RubiconRouter, and validate during cancel

[bghughes]

Not yet used in production, good issue!

[HickupHH3]

duplicate of #17