search

liquity / bold

Liquity v2 monorepo containing the contracts, subgraph and frontend.
https://www.liquity.org/liquity-v2
44 stars 12 forks source link

CS-BOLD-008 Low 5.11: Opening Troves Can Be Blocked #472

Open bingen opened 1 month ago

bingen commented 1 month ago

The function TroveManager._openTrove computes the trove ID from the owner address and a trove index chosen by the caller. Anyone can open a trove on behalf of another _owner by providing the necessary collateral. Note that there cannot be two troves with the same trove ID:

vars.troveId = uint256(keccak256(abi.encode(_owner, _ownerIndex)));
_requireTroveIsNotOpen(vars.troveManager, vars.troveId);

An attacker can grief other users by frontrunning them and opening a trove with the same ID on behalf of the same account, setting themselves as the manager and receiver of the collateral. They could then backrun the failing transaction by withdrawing the collateral from the trove. This attack is not free, as the attacker must pay the upfront fee for the trove. However, the fee can be relatively small. At an average interest rate of 5%, they would pay 0.05 * 2000 / 52 = 1.92 BOLD, plus the gas costs for the operation. The owner of the trove can remove the attacker as manager and claim their collateral if they are able to make a transaction before the attacker. This griefing is most problematic for multisigs or governance proposals that are executed after a time lock. Here, an attacker can potentially permanently DOS a contract from opening troves.

bingen commented 1 month ago

I don’t think this is going to be a problem, but if it is we could have workarounds in the form of zappers. One could be to first check if the trove exists, and if it does first close it. That would prevent the attack and even make the owner make some money. If we finally merge PR #522 for making trove ids non-reusable, this becomes worse, as this workaround wouldn’t work. But we could still do something like using TroveIds.length + 1 as the index. So we won’t fix it at core level.

Of course there’s also the possibility of using things like Flashbots.

bingen commented 1 month ago

As pointed out by ChainSecurity, in some special cases like a DAO with a timelock, those measures may not be enough:

  1. Some DAO votes that they want to create a Bold trove with their treasury.
  2. The proposal must have the function call that is being voted on hardcoded ahead of time.
  3. There is a timelock after the vote passes before the tx can be executed.

In this case I don't think either of the suggested mitigations in the issue (an extra zapper or flashbots) would help.

The griefer would not delegate to the zapper, so only the owner can close the trove (but the owner is a timelock, so cannot react). And the griefer has a lot of time (timelock cooldown) to create the griefing trove, they don't need to directly frontrun the execution. So flashbots doesn't help.

For the TroveIds.length + 1, The griefer can just use troveids.length + 2 as index

But we still think it will be possible to come up with some mitigation. Some measures could be used, like trying a set of indexes instead of only 1 (to make it more expensive), use the block number, so the attacker has to frontrun, use msg.sender and whitelist, leave index as a parameter chosen by the executor along with try-catch...

Anyway, I don’t think that’s a very common use case. A rigid process like that type of Timelock doesn’t seem a good fit for a trove, as it wouldn’t be agile to react to ETH price changes to repay, or to adjust interest rate.