CIP 34 - Implement "flash swap" support in Exchange.sol - Discussions

[YazzyYaz]

trafficstars

Discussions to CIP-34

[YazzyYaz]

Comment by @prestwich "Generally speaking, I'm in favor of this. I'd like to see more specification (should we follow the uniswap interface precisely? can we achieve the same guarantees uniswap does?)"

[zviadm]

Comment by @prestwich "Generally speaking, I'm in favor of this. I'd like to see more specification (should we follow the uniswap interface precisely? can we achieve the same guarantees uniswap does?)"

Uniswap design is different so that exact interface probably won't work. Uniswap stuff is just a good reference for implementation.

Exchange.sol version will probably just replace internal _exchange function with new interface:

private _exchange(uint256 sellAmount, uint256 buyAmount, bool sellGold, address to, bytes calldata data) {
  // this function will now optimistically transfer `buyAmount` to `to` address first,
  // then call something like: ExchangeCallee(to).exchangeCall(msg.sender, ....)
  // then at the end transfer `sellAmount` in.
}

and will need to add one new public function that is really only intended to be called by other contracts:

public exchangeDirect(uint256 sellAmount, bool sellGold, address to, bytes calldata data) {
  // this will calculate the buyAmount
  // call internal _exchange function to do the exchange.
  //
  // Notice that this function doesn't have any safeguards, it is callers responsibility to make sure that they will
  // get 'buyAmount' that they are expecting. Since exchangeDirect is expected to be called by a smart contract
  // they can do all the checks transactionally.
}

exchangeDirect can be called something else too. Some other options:

• exchangeRaw?
• exchangeUnsafe?
• or some better name that describes that this is an advanced function and you should use it carefully

[zviadm]

Thanks for the comments everyone in todays call.

As I understand we can separate discussion into two high level parts:

1. Should Mento allow regular trading between CELO/cUSD or not in general?
2. Assuming Mento remains as is, should we implement this CIP 34 or not.

First question is a completely separate topic so I think unless there are already plans to significantly change the way Mento behaves, it might be more productive to just focus on the second question as is.

[nategraf]

I'd like to add a security consideration. Although in most respects attacks which can be done with a flash swap would be done without it given sufficient capital, this change does break the invariant that the total "value" of the buckets remain constant. I'm not immediately aware of an attack, but breaking this invariant should be considered in our security analysis. An extreme example of this is that, IIUC, an attacker could completely empty one of the buckets (or down to 1 wei) and then call into another contract which attempts to trade with the exchange, but would be potentially vulnerable in new ways do to the fact that one of the buckets is (almost) empty.

Another consideration is whether this could be used to abuse the privileges of the Exchange contract. If I understand correctly, a naive implementation of the callback could be used to direct a call to the Reserve from the Exchange, which is an authorized spender. Is there any good way to prevent this?

[zviadm]

I'd like to add a security consideration. Although in most respects attacks which can be done with a flash swap would be done without it given sufficient capital, this change does break the invariant that the total "value" of the buckets remain constant. I'm not immediately aware of an attack, but breaking this invariant should be considered in our security analysis. An extreme example of this is that, IIUC, an attacker could completely empty one of the buckets (or down to 1 wei) and then call into another contract which attempts to trade with the exchange, but would be potentially vulnerable in new ways do to the fact that one of the buckets is (almost) empty.

Re: first point: internal _exchange function should be non-reentrant, so it won't actually be possible to call another thing that tries to exchange while you are in the middle of an exchange. This is pretty important because this way, exchange doesn't really allow 'flash loans', it only allows flash swaps, so you actually need to complete the swap for transaction to succeed.

It is of course possible to cause issues if there are contracts that depend on instantaneous bucket sizes. But those contracts will be vulnerable anyways. Current CELO bucket size is <400K. Assuming any of the DeFI projects are moderately successful (i.e. Moola or Ubeswap), they will have enough liquidity and also flash loan/flash swap support to also temporarily shift bucket sizes to any value (including depleting to just 1 wei too). Flash loan/flash swap costs are 0.6-0.9% so it is very cheap to do at 400K CELO scale.

Another consideration is whether this could be used to abuse the privileges of the Exchange contract. If I understand correctly, a naive implementation of the callback could be used to direct a call to the Reserve from the Exchange, which is an authorized spender. Is there any good way to prevent this?

These are type of questions where I would prefer to defer to more experienced solidity developers. My understanding is that, callback is calling a 'specific' function, so you can't trick it to call a random function from a specific contract, and after the call is made, msg.sender will become the calling contract so they wont be able to impersonate 'Exchange' contract. But I am not 100% confident in all soldity security details when it comes to more complex callbacks, thus would definitely defer to others on this.