search

Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

BaseRouter protections #204

Closed 0xdcota closed 1 year ago

0xdcota commented 1 year ago

@brozorec this PR is ready for review.

I implemented some checks on xReceive call that maybe make calls permissioned per Connext rules. Though, I couldn't figure out another simpler way to do it. We can brainstorm together or I can walk you through it.

There may be some refactoring possible too; and I am happy to implement them. Though I think the PR is at a point for another set of eyes.

brozorec commented 1 year ago

I see in this PR 3 types of changes

@DaigaroCota

0xdcota commented 1 year ago

I see in this PR 3 types of changes

  • checking beneficiary: resolves 5.10 and 5.11 of the report
  • allowCaller: makes the call to the router permissioned but I'm not sure what issue it addresses
  • noBalance check: extends the checks for all tokens from all actions. You're right that we can think of refactoring because there are some pure functions that can be exported to a library (as a first idea).

@DaigaroCota

allowCaller: helps idenitfy receiving calls from specific bridges to perform specific checks.

  1. In the ConnextRouter, these two checks work together. The first check ensures that Connext is the caller. The second one (now granted that Connext is the caller) overrides the _checkNoBalanceChange() checks to allow a deposit.

https://github.com/Fujicracy/fuji-v2/blob/523fa9c653ffbf0320dca78572ffcf0e2f311d4d/packages/protocol/src/routers/ConnextRouter.sol#L106-L119

Otherwise, if you don't check that Connext is the caller, then any caller could steal stuck funds in the ConnextRouter contract.

brozorec commented 1 year ago

@DaigaroCota Sure, I see what it does but I don't understand the problem it tries to resolve. Can you describe a case where this check isn't done and this leads to a potential exploit?

I think we can't leave this call to be a permissioned one because this means a tx will take the slow path and it will need 1-2h to settle which I consider a killer.

0xdcota commented 1 year ago

@DaigaroCota Sure, I see what it does but I don't understand the problem it tries to resolve. Can you describe a case where this check isn't done and this leads to a potential exploit?

I think we can't leave this call to be a permissioned one because this means a tx will take the slow path and it will need 1-2h to settle which I consider a killer.

I added the attack case to showcase. First comment out these lines in the ConnextRouter.sol: https://github.com/Fujicracy/fuji-v2/blob/523fa9c653ffbf0320dca78572ffcf0e2f311d4d/packages/protocol/src/routers/ConnextRouter.sol#L107-L109

then run:

forge test --mp test/forking/goerli/ConnextRouter.t.sol --mt test_attackXReceive-vv

You should get the following logs:

Logs:
  attack succeeded
  Error: a == b not satisfied [uint]
    Expected: 0
      Actual: 2000000000000000000
  Error: a == b not satisfied [uint]
    Expected: 0
      Actual: 1000000000

The attack consists on taking advantage of previously stuck funds, and steal them, using the xReceive as an entry point.

As an additional note, if you remove the following lines: https://github.com/Fujicracy/fuji-v2/blob/523fa9c653ffbf0320dca78572ffcf0e2f311d4d/packages/protocol/src/routers/ConnextRouter.sol#L112-L119

then you fail the test_bridgeInbound due to checks failing because of the balance un-awareness when funds are pushed to the ConnextRouter.

brozorec commented 1 year ago

Ok, I see; thanks for the explanation :+1:

However, this attack can happen if the attacker, instead of calling xReceive calls xBundle.

On a more general note, we have to look at this from another angle and try to solve it by avoiding putting permissions. In order to prevent such kinds of exploits, we shouldn't let any leftover funds at the end of a transaction.

  1. If the xReceive call fails, we might transfer those funds to a rescue contract where we handle them accordingly.
  2. If some funds are left in the Router at the end of a tx for whatever reason, we transfer them to the beneficiary.
  3. If a user sends funds to the Router with a simple "transfer", we consider it a user error.

@DaigaroCota

0xdcota commented 1 year ago

However, this attack can happen if the attacker, instead of calling xReceive calls xBundle.

Not really, because when calling xBundle it will revert when checking erc20-balance of this.router prior to internalBundle is not = to balance at the end of internalBundle.

At the xReceive entry point this check is overriden, and the reason is because funds are pushed to the router.

0xdcota commented 1 year ago

On a more general note, we have to look at this from another angle and try to solve it by avoiding putting permissions. In order to prevent such kinds of exploits, we shouldn't let any leftover funds at the end of a transaction.

  1. If the xReceive call fails, we might transfer those funds to a rescue contract where we handle them accordingly.
  2. If some funds are left in the Router at the end of a tx for whatever reason, we transfer them to the beneficiary.
  3. If a user sends funds to the Router with a simple "transfer", we consider it a user error.

I agree, however, I think these implementations are not a "3 day fix" ticket issue. In fact I think this is one of the critical path problems we have on hand in FujiV2. I suggest we merge this PR as is, with awareness that we are making permissioned calls, and that we start a new issue ticket or epic with the challenge to remove the permission.

Also, that we have a call session to explore possibilities, and specify these types of ideas further.

0xdcota commented 1 year ago

@brozorec as discussed in today's meeting, lets proceed with permissioned and I will create a new issue-task to start the exploring the Holder.sol contract solution.

0xdcota commented 1 year ago

I added protection against in ConnextRouter against entry point both via xReceive and xBundle and demonstrated in test_attackXReceive() as discussed.