Submitted by shalaamum, also found by nirlin, Lirios, carrotsmuggler, BGSecurity, Inspex, DedOhWale, Lirios, tallo, nobody2018, and T1MOH.
https://github.com/code-423n4/2023-04-rubicon/blob/511636d889742296a54392875a35e4c0c4727bb7/contracts/RubiconMarket.sol#L900 https://github.com/code-423n4/2023-04-rubicon/blob/511636d889742296a54392875a35e4c0c4727bb7/contracts/RubiconMarket.sol#L926
The RubiconMarket functions batchOffer and batchRequote both contain calls to offer as follows:
But calling offer using this.offer will use a message call instead of a jump. This means that in offer, the value of msg.sender will be the address of the RubiconMarket contract, not of the original sender. Hence, the offer will be made by the RubiconMarket itself.
An attacker can use this to make RubiconMarket offer all its balance of a particular token (possibly in exchange for a token the attacker created, at effectively zero cost), with the attacker then taking the offer and thereby stealing RubiconMarkets entire balance for that token.
For this attack to work, the RubiconMarket must be able to actually make the offer itself; the crucial condition here is in line 538.
In the offer function of SimpleMarket, it must not revert if this call is made by the RubiconMarket contract and msg.sender is also the same contract, even though RubiconMarket did not approve a transfer to itself.
Whether this condition is satisfied will depend on the token.
For example, in the standard ERC20 token contract by OpenZeppelin, this condition is not satisfied. However, in the WETH contract, the check in the transferFrom function is.
If the caller is also the src, then it is not necessary to approve the transfer beforehand. This means the vulnerability outlined above can be used to drain all WETH held by a RubiconMarket, and possibly the balance in other tokens where transferFrom is implemented similarly.
After applying the diff below, run:
To run the POC, the output should contain the following:
The POC demonstrates how an attacker can deploy their own token and then use batchOffer to make the RubiconMarket offer its entire balance of WETH for some of the attackers token. The attacker then takes that offer at no cost (except some gas) and thereby steals the 10 WETH the RubiconMarket held in escrow for previous unfilled offers.
Apply the following diff to test/hardhat-tests/ProtocolDeployment.ts:
Change this.offer to offer. As the offer function is currently external, it will have to be changed to public.
bghughes (Rubicon) confirmed
HickupHH3 (judge) commented:
