Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict Deployment of Matching Quote and Collateral tokens #735

Merged
merged 3 commits into from
Apr 9, 2023

Conversation

ith-harvey
Copy link
Contributor

@ith-harvey ith-harvey commented Apr 8, 2023

Description of change

High level

  • added a revert in the canDeploy pool modifier: DeployQuoteCollateralSameToken().
  • The revert fires if the quote and collateral addresses are equal to one another.

Description of bug or vulnerability and solution

  • Protocol allowed for pools that had the same quote and collateral token.
    • Not a bug but an undesired user experience and could result in UI confusion / error

Contract size

Pre Change

  ERC721PoolFactory        -   3,340B  (13.59%)
  ERC20PoolFactory         -   2,473B  (10.06%)

Post Change

  ERC721PoolFactory        -   3,390B  (13.79%)
  ERC20PoolFactory         -   2,523B  (10.27%)

Gas usage

Pre Change

| src/ERC20PoolFactory.sol:ERC20PoolFactory contract |                 |        |        |        |         |
|----------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                    | Deployment Size |        |        |        |         |
| 5393302                                            | 26753           |        |        |        |         |
| Function Name                                      | min             | avg    | median | max    | # calls |
| deployPool                                         | 602             | 226907 | 232241 | 255710 | 252     |
| deployedPools                                      | 854             | 2417   | 2854   | 2854   | 55      |
| deployedPoolsList                                  | 703             | 703    | 703    | 703    | 3       |

| src/ERC721PoolFactory.sol:ERC721PoolFactory contract |                 |                   |        |                     |         |
|------------------------------------------------------|-----------------|-------------------|--------|---------------------|---------|
| Deployment Cost                                      | Deployment Size |                   |        |                     |         |
| 5692525                                              | 28251           |                   |        |                     |         |
| Function Name                                        | min             | avg               | median | max                 | # calls |
| deployPool                                           | 926             | 66202914522633298 | 303974 | 8937393460516718734 | 135     |
| deployedPools                                        | 898             | 2461              | 2898   | 2898                | 55      |
| deployedPoolsList                                    | 614             | 614               | 614    | 614                 | 1       |

Post Change

| src/ERC20PoolFactory.sol:ERC20PoolFactory contract |                 |        |        |        |         |
|----------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                    | Deployment Size |        |        |        |         |
| 5403305                                            | 26803           |        |        |        |         |
| Function Name                                      | min             | avg    | median | max    | # calls |
| deployPool                                         | 603             | 226105 | 232300 | 255769 | 254     |
| deployedPools                                      | 854             | 1970   | 2854   | 2854   | 77      |
| deployedPoolsList                                  | 703             | 703    | 703    | 703    | 3       |

| src/ERC721PoolFactory.sol:ERC721PoolFactory contract |                 |                   |        |                     |         |
|------------------------------------------------------|-----------------|-------------------|--------|---------------------|---------|
| Deployment Cost                                      | Deployment Size |                   |        |                     |         |
| 5702528                                              | 28301           |                   |        |                     |         |
| Function Name                                        | min             | avg               | median | max                 | # calls |
| deployPool                                           | 927             | 64297794680261978 | 304033 | 8937393460516718735 | 139     |
| deployedPools                                        | 898             | 2014              | 2898   | 2898                | 77      |
| deployedPoolsList                                    | 614             | 614               | 614    | 614                 | 1       |

@ith-harvey ith-harvey changed the title initial commit Restrict Deployment of Matching Quote and Collateral tokens Apr 8, 2023
@ith-harvey ith-harvey marked this pull request as ready for review April 8, 2023 18:25
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue with testDeployERC721CollectionPoolWithInvalidRate test. Factory changes look good.

tests/forge/unit/ERC721Pool/ERC721PoolFactory.t.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@ith-harvey ith-harvey merged commit 0c64f0e into develop Apr 9, 2023
@ith-harvey ith-harvey deleted the restrict-same-token-pools branch April 11, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants