sherlock-audit / 2023-12-arcadia-judging

18 stars 15 forks source link

PUSH0 - `CREATE2` address collision against an Account will allow complete draining of lending pools #59

Open sherlock-admin2 opened 7 months ago

sherlock-admin2 commented 7 months ago

PUSH0

high

CREATE2 address collision against an Account will allow complete draining of lending pools

Summary

The factory function createAccount() creates a new account contract for the user using CREATE2. We show that a meet-in-the-middle attack at finding an address collision against an undeployed account is possible. Furthermore, such an attack allows draining of all funds from the lending pool.

Vulnerability Detail

The attack consists of two parts: Finding a collision, and actually draining the lending pool. We describe both here:

PoC: Finding a collision

Note that in createAccount, CREATE2 salt is user-supplied, and tx.origin is technically also user-supplied:

account = address(
    new Proxy{ salt: keccak256(abi.encodePacked(salt, tx.origin)) }(
        versionInformation[accountVersion].implementation
    )
);

The address collision an attacker will need to find are:

Both sets of addresses can be brute-force searched because:

An attacker can find any single address collision between (1) and (2) with high probability of success using the following meet-in-the-middle technique, a classic brute-force-based attack in cryptography:

The feasibility, as well as detailed technique and hardware requirements of finding a collision, are sufficiently described in multiple references:

The hashrate of the BTC network has reached $6 \times 10^{20}$ hashes per second as of time of writing, taking only just $33$ minutes to achieve $2^{80}$ hashes. A fraction of this computing power will still easily find a collision in a reasonably short timeline.

PoC: Draining the lending pool

Even given EIP-3607 which disables an EOA if a contract is already deployed on top, we show that it's still possible to drain the lending pool entirely given a contract collision.

Assuming the attacker has already found an address collision against an undeployed account, let's say 0xCOLLIDED. The steps for complete draining of a lending pool are as follow:

First tx:

The attacker now has complete control of any funds sent to 0xCOLLIDED.

Second tx:

The attacker has stolen all funds from the lending pool.

Coded unit-PoC

While we cannot provide an actual hash collision due to infrastructural constraints, we are able to provide a coded PoC to prove the following two properties of the EVM that would enable this attack:

Here is the PoC, as well as detailed steps to recreate it:

  1. Paste the following file onto Remix (or a developing environment of choice): https://gist.github.com/midori-fuse/087aa3248da114a0712757348fcce814
  2. Deploy the contract Test.
  3. Run the function Test.test() with a salt of your choice, and record the returned address. The result will be:
    • Test.getAllowance() for that address will return exactly APPROVE_AMOUNT.
    • Test.getCodeSize() for that address will return exactly zero.
    • This proves the second property.
  4. Using the same salt in step 3, run Test.test() again. The tx will go through, and the result will be:
    • Test.test() returns the same address as with the first run.
    • Test.getAllowance() for that address will return twice of APPROVE_AMOUNT.
    • Test.getCodeSize() for that address will still return zero.
    • This proves the first property.

The provided PoC has been tested on Remix IDE, on the Remix VM - Mainnet fork environment, as well as testing locally on the Holesky testnet fork, which as of time of writing, has been upgraded with the Dencun hardfork.

Impact

Complete draining of a lending pool if an address collision is found.

With the advancement of computing hardware, the cost of an attack has been shown to be just a few million dollars, and that the current Bitcoin network hashrate allows about $2^{80}$ in about half an hour. The cost of the attack may be offsetted with longer brute force time.

For a DeFi lending pool, it is normal for a pool TVL to reach tens or hundreds of millions in USD value (top protocols' TVL are well above the billions). It is then easy to show that such an attack is massively profitable.

Code Snippet

https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/Factory.sol#L96-L100

Tool used

Manual Review, Remix IDE

Recommendation

The mitigation method is to prevent controlling over the deployed account address (or at least severely limit that). Some techniques may be:

This will prevent brute-forcing of one side of the collision, disabling the $O(2^{81})$ search technique.

sherlock-admin2 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: high(5)

nevillehuang commented 7 months ago

Request PoC to facilitate discussion between sponsor and watson.

I believe this is low severity given the extreme unlikeliness of it occuring, with the addition that a insanely huge amount of funds is required to perform this attack without guarantee

sherlock-admin commented 7 months ago

PoC requested from @PUSH0

Requests remaining: 8

midori-fuse commented 7 months ago

Hello, thanks for asking.

The reason we believe this is a valid HM is the following:

Therefore the likelihood of this attack can only increase as time passes. Complete draining of a pool also cannot be low severity.

Essentially by identifying this attack, we have proven the existence of a time bomb, that will allow complete draining of a certain pool from any given chain. We understand that past decisions are not considered a source of truth, however the issue discussion and the external resources should still provide an objective reference to determine this issue's validity. Furthermore let's consider the impact if it were to happen as well.

nevillehuang commented 7 months ago

@Czar102 I am interested in hearing your opinion here with regards to the previous issue here as well.

Thomas-Smets commented 7 months ago

Don't think it is a high, requires a very big upfront cost with no certainty on pay out as attacker. And while our factory is immutable, we can pause indefinitely the creation of new accounts.

If at some point the attack becomes a possibility, we can still block it.

To make it even harder, we are thinking to add the block.timestamp and block.number to the hash. Then the attacker, after they successfully found a hash collision, already has to execute the attack at a fixed block and probably conspire with the sequencer to ensure that also the time is fixed.

midori-fuse commented 7 months ago

Agree that it's not a high, the attack cost makes a strong external condition.

Re: mitigations. I don't think pausing the contract or blocking the attack makes sense, the attack would sort of finish in a flash before you know it (and in a shorter duration than a pausing multisig can react).

Regarding the fix, adding block.timestamp and block.number technically works, but doesn't really make sense as opposed to just using the vanilla creation. The main purpose of CREATE2 is that contract addresses are deterministic and pre-computable, you can do a handful of things with this information e.g. funds can be sent there in advance before contract creation, or crafting a customized address without deploying. By adding block number and timestamp, the purpose is essentially defeated.

But since it technically works, there's not really a point in opposing it I suppose. A full fix would involve reworking the account's accounting, which I think is quite complex and may open up more problems.

A fix that would still (partially, but should be sufficient) retain the mentioned functionalities could be just using a uint64 salt, or the last 64 bits of the address only, or anything that doesn't let the user determine more than 64 bits of the input. Then the other side of the brute force has to achieve $2^{15} = 32768$ times the mentioned hashing power in the attack to achieve a sizeable collision probability (and such probability is still less than a balanced $2^{80}$ brute force anyway).

Thomas-Smets commented 7 months ago

I don't think pausing the contract or blocking the attack makes sense

Meant it in the sense that if such an attack occurs we can pause it, assuming we would not be the first victim. Not that we can frontrun a particular attack with a pause.

Regarding the fix, adding block.timestamp and block.number technically works, but doesn't really make sense as opposed to just using the vanilla creation.

Fair point.

A fix that would still (partially, but should be sufficient) retain the mentioned functionalities could be just using a uint64 salt, or the last 64 bits of the address only, or anything that doesn't let the user determine more than 64 bits of the input.

I do like this idea thanks! We can use 32 bits from the tx.origin and 32 bits from a salt.

sherlock-admin commented 7 months ago

The protocol team fixed this issue in PR/commit https://github.com/arcadia-finance/accounts-v2/pull/176.

IAm0x52 commented 6 months ago

Fix looks good. By using a 32 bit salt the collision is increased from 2^81 to 2^128 dramatically increasing the cost.

sherlock-admin4 commented 6 months ago

The Lead Senior Watson signed off on the fix.