[BIP-299] Permissioned arbitrage to recover remaining funds in bb-e-usd

Governance
4

Hi everyone, the Integrations Team also reviewed this last week. @joaobrunoah, @gerg, and myself each put eyes on the code. We agree that the do_arb function, in the context of the specified Gnosis transaction batch, is safe to execute. We also had the following comments which amount to an informational QA report; none of the issues mentioned has a material impact on execution when viewed in the context of this specific payload. Line numbers and function names refer to the bbeUSD_arb contract.

Trust Assumptions

All external functions are marked onlyOwner, so only the current owner or any future owner (after ownership transfer) has the power to execute. If we trust the current owner to properly execute and not to transfer ownership to a malicious entity, then we can trust the contract.

Quality Assurance Report

  • The do_arb function will fail for all pools if any one of the provided inputTokens[i] has a 0 balance or doesn’t contain the specified outputTokens[i].
  • For governance transparency, the recipient address (do_arb) and payee addresses (withdraw and sweep) should be immutable: either hard-coded or specified at construction time.
  • There is a comment implying that this is a single-use contract but nothing to enforce it.
  • The sweep function should utilize safeTransfer to account for tokens like USDT (out of scope for the specified payload).
  • The deadline (block.timestamp + 30) on line 43 represents an infinite deadline. To specify a real deadline, the timestamp must be passed into this contract from off chain. For an infinite deadline, prefer something like type(uint256).max for clarity.
  • <payable>.transfer(amount) can fail. It is preferable to use (bool sent, bytes memory data) = <payable>.call{value: amount}, but if the payee is expected to be an EOA or a contract with minimal receive logic, it is fine.
  • do_arb needs more comments (NatSpec) to explain what the function does and the main parts of its code. It’s unclear when reading it and even harder to understand if the contract does what the author intended, because author’s intentions are not documented in the code.
  • inputTokens should be renamed to linearPools or similar to distinguish it from a generic array of ERC-20 tokens.
  • lt (line 25) should be renamed to linearPool or another more descriptive name.
  • usdToken (line 26) is not used and should be removed.
  • foo (line 27) is not used and should be removed.
  • 10 ** 50 (line 43) seems arbitrary and could be replaced by type(uint256).max.
  • outputTokens is unnecessary because we already validate it against linearPool.mainToken(). Just use that getter directly.
  • dustFactors is a strange way to fuzz the output — could alternatively calculate the amount of fees the pool will collect as it’s drained.
  • The payee in the internal_sweep function does not need to be payable since it does not handle ETH.
  • The @author tag misspells one of the author’s names.

Gas Optimizations

  • inputTokens, outputTokens and dustFactor can be calldata instead of memory.
  • In the for loops, change i++ to ++i.
  • Instead of calling lt.getPoolId() 3 times, use the poolId variable from line 38.
  • The whole do_arb function can be rewritten to use a single batchSwap instead of multiple swaps:
before loop:
    create empty array of assets (IAssets)
    create empty array of swaps (BatchSwapStep)
    create empty array of limits (uint256)
in loop:
    add in/out tokens to assets array
    populate BatchSwapStep array w/ indices of in/out tokens
    populate limits
after loop:
    populate FundManagement struct
    set deadline (see comment above about block.timestamp being useless as a deadline)
    execute batchSwap
9 Likes