dfx-finance/protocol-v2

A decentralized foreign exchange protocol optimized for stablecoins.

protocol-v2's Issues

Inclusion of ToB audit report is misleading

In light of the recently published post-mortem, I was reviewing the existing audit coverage for this version of the protocol and saw that the Trail of Bits audit is included (it was imported with the rest of the audits in #73).

Having read the ToB audit when it was first published, I had assumed it was for V1. Slightly confused, I attempted to retrieve the commit specified in the report (906bd5274dcd07c458e6bbd6f13adced873ac952), only to realise it really was for the V1 repo.

Whilst more communication is certainly better when it comes to security audit coverage, I think this communication would benefit from more clarity surrounding what exactly has been audited by who. I suppose the easiest fix would be to create subdirectories for each protocol version and have the relevant reports contained within those?

Unused Contracts

• check for unused contracts
• remove unused contracts

1 - The whitelisting option is always turned off

3.3

5 - newCurve() needs more input validation

5.2

Update dependancies

• make sure that the whole file is not being used but a git reference

2.4

LPT Price Retain Test Port

• Port to forge
• Fuzzing

Invariant Test Port

• Port to forge
• Fuzzing

Repeated USDC-USD assimilator deployment

https://github.com/dfx-finance/protocol/blob/v2/contracts/CurveFactoryV2.sol#L99-#L102
Curve Factory V2 deploys a new USDC-USD assimilator with every new curve. Should change this because of it would cost a lot of money redeploying the same contract every time.

11 - Assimilator’s functions can be simplified, and improve on precision as well

Audit Summary Checklist

• Issue #36 (Oracle Front Run):
  PR: Will set up AMM infra post audit see discussion here new issue here

• Issue #37 (Missing Proxy Protection)
  PR here

• Issue #38 (DepositWithWhitelist Validation)
  PR here

• Issue #39 (Unclear Error)
  PR here

• Issue #40 (Emergency State Clarification)
  PR here

• Issue #41 (Transactable Check)
  PR: Is intentional see discussion here

• Issue #42 (Protected Function)
  PR here

• Issue #43 (Verification)
  PR here

• Issue #44 (newAssimilator Definition)
  PR here

• Issue #45 (Overwriting Curve)
  PR here

• Issue #46 (Potential Time Lag)
  PR here

• Issue #47 (Unnecessary Function)
  PR here

• Issue #48 (Library Update)
  PR here

• Issue #49 (Currency Check)
  PR here

• Issue #50 (Equivalent Function)
  PR here

Polygon Foundry Deployment Issue

There are problems with deploying ANY contracts through foundry's forge create as well as forge script both don't work under any RPC i.e. alchemy. I even tested with multiple gas multipliers with no luck. Will need to open up an issue with foundry. Current work around will use a hardhat project to deploy. Mainnet should be working fine as I have tested on Goerli.

[] Opening up foundry issue for polygon ProviderError(JsonRpcError(JsonRpcError { code: -32000, message: "transaction underpriced", data: None }))

4.1

Assimilator Test

7.1

2.3

2.5

Update Readme

• deploy locally
• testing
• Descriptions of functions / documentations

12 - Incorrect calculation of _swapInfo.totalFees may lead to excessive fees for the user

9 - AssimilatorFactory’s transferFee() method does not use their return parameter

3.1

Router Test Port

CI - github actions

Using maple finance github actions is a good idea
ref: https://github.com/maple-labs/erc20/tree/main/.github/workflows

1.1

8.1

Set up a bot to report new curves

• curve address
• assimilator address
• oracle used
• token used

Factory Test Port

Txn monitoring for oracles with lower thresholds

Protocol Fee Test Port

• Port to forge
• Fuzzing

Swap Difference Test Port

• Port to forge
• Fuzzing

Flashloans

After some research probably should be using uniswapv3 code. Angle and aave doesn't fit what we're looking for i.e. mint flashloans.

3 - unzap() has no MEV protection

4 - LP withdrawal may not work if the minter is not the withdrawer

Get Curve Discrepancy

Old CurveFactory Contract

function getCurve() encodes id as bytes32 curveId = keccak256(abi.encode(_baseCurrency, _quoteCurrency));
which aligns with
newCurve() encoding id as bytes32 curveId = keccak256(abi.encode(_baseCurrency, _quoteCurrency));
ref: https://github.com/dfx-finance/protocol/blob/main/contracts/CurveFactory.sol#L31-L46

New CurveFactoryV2 Contract

function getCurve() only has one param bytes32 curveId = keccak256(abi.encode(_token));
which DOES NOT align with
newCurve encoding with old id as bytes32 curveId = keccak256(abi.encode(_info._baseCurrency, _info._quoteCurrency));
ref: https://github.com/dfx-finance/protocol/blob/v2/contracts/CurveFactoryV2.sol#L85-L91

Port over remaining contracts

• solidity v0.8.0
• forge build

CI - Slither

Slither needs to ignore some of the dependancies inorder to work well with github actions

7 - Front-Running on pool guard amount setting

CI/CD

Hardfork tests don't work because the infura API is no longer supported. Will have to store in github actions.

10 - oracle update can be sandwiched to extract value from the LP, if fee parameter is incorrectly set

2 - newCurve() cause DoS by Front-Running

6

Curve Generalization Test Port

• Port to forge
• Fuzzing

5.1

6.1

2.2

2.1

8 - lack of zero address check for factory address

3.2