code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L378-L513

function calculateAddLiquidityQuantities(
    uint256 _baseTokenQtyDesired,
    uint256 _quoteTokenQtyDesired,
    uint256 _baseTokenQtyMin,
    uint256 _quoteTokenQtyMin,
    uint256 _baseTokenReserveQty,
    uint256 _quoteTokenReserveQty,
    uint256 _totalSupplyOfLiquidityTokens,
    InternalBalances storage _internalBalances
) public returns (TokenQtys memory tokenQtys) {
    // ...
}

Can be changed to:

function calculateAddLiquidityQuantities(
    uint256 _baseTokenQtyDesired,
    uint256 _quoteTokenQtyDesired,
    uint256 _baseTokenQtyMin,
    uint256 _quoteTokenQtyMin,
    uint256 _baseTokenReserveQty,
    uint256 _quoteTokenReserveQty,
    uint256 _totalSupplyOfLiquidityTokens,
    InternalBalances memory _internalBalances
) public returns (TokenQtys memory tokenQtys, InternalBalances memory updatedInternalBalances) {
    // ...
}

In Exchange#addLiquidity(), change internalBalances storage to updatedInternalBalances:

remove storage parameter in library function to avoid storage modification outside the contract code;
make it easier to use calculateAddLiquidityQuantities() in view functions;
avoid duplicated read/write to storage in calculateAddLiquidityQuantities() to save gas.

GalloDaSballo commented 2 years ago

I believe the warden finding to be valid, although nuanced. The pointer to storage as parameter is not necessarily more expensive (still up to 32 bytes), however reads to storage are, so if the warden can find a way to pass memory / calldata values, the reduction is SLOADs should provide gas savings

0xean commented 2 years ago

This actually increases gas costs.

Before

·---------------------------------------------------------|---------------------------|----------------|-----------------------------·
|                   Solc version: 0.8.4                   ·  Optimizer enabled: true  ·  Runs: 100000  ·  Block limit: 12450000 gas  │
··························································|···························|················|······························
|  Methods                                                                                                                           │
···························|······························|·············|·············|················|···············|··············
|  Contract                ·  Method                      ·  Min        ·  Max        ·  Avg           ·  # calls      ·  usd (avg)  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  approve                     ·      46184  ·      46196  ·         46185  ·           81  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  simulateRebaseDown          ·      53489  ·      53525  ·         53506  ·           16  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  transfer                    ·      19327  ·      51439  ·         47144  ·          102  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply  ·  approve                     ·      46172  ·      46196  ·         46185  ·           84  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply  ·  transfer                    ·      19360  ·      51472  ·         49630  ·           89  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  addLiquidity                ·      92980  ·     240475  ·        196335  ·           72  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  approve                     ·          -  ·          -  ·         46200  ·            2  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  removeLiquidity             ·      51261  ·     174468  ·         96283  ·           41  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  swapBaseTokenForQuoteToken  ·      82054  ·      99154  ·         97839  ·           13  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  swapQuoteTokenForBaseToken  ·      71147  ·     101159  ·         94316  ·           24  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ExchangeFactory         ·  createNewExchange           ·    2490989  ·    2491073  ·       2491003  ·            6  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ExchangeFactory         ·  setFeeAddress               ·          -  ·          -  ·         30276  ·            3  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  FeeOnTransferMock       ·  approve                     ·          -  ·          -  ·         46196  ·            1  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  FeeOnTransferMock       ·  transfer                    ·          -  ·          -  ·         59045  ·            1  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Deployments                                            ·                                            ·  % of limit   ·             │
··························································|·············|·············|················|···············|··············
|  ElasticMock                                            ·          -  ·          -  ·       1130216  ·        9.1 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply                                 ·          -  ·          -  ·        924077  ·        7.4 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  Exchange                                               ·          -  ·          -  ·       2609521  ·         21 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  ExchangeFactory                                        ·          -  ·          -  ·       3441325  ·       27.6 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  FeeOnTransferMock                                      ·          -  ·          -  ·       1186152  ·        9.5 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  MathLib                                                ·          -  ·          -  ·       1351274  ·       10.9 %  ·          -  │
·---------------------------------------------------------|-------------|-------------|----------------|---------------|-------------·

After


·---------------------------------------------------------|---------------------------|----------------|-----------------------------·
|                   Solc version: 0.8.4                   ·  Optimizer enabled: true  ·  Runs: 100000  ·  Block limit: 12450000 gas  │
··························································|···························|················|······························
|  Methods                                                                                                                           │
···························|······························|·············|·············|················|···············|··············
|  Contract                ·  Method                      ·  Min        ·  Max        ·  Avg           ·  # calls      ·  usd (avg)  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  approve                     ·      46184  ·      46196  ·         46185  ·           81  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  simulateRebaseDown          ·      53489  ·      53525  ·         53506  ·           16  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ElasticMock             ·  transfer                    ·      19327  ·      51439  ·         47144  ·          102  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply  ·  approve                     ·      46172  ·      46196  ·         46185  ·           84  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply  ·  transfer                    ·      19360  ·      51472  ·         49630  ·           89  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  addLiquidity                ·      93063  ·     241339  ·        197000  ·           72  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  approve                     ·          -  ·          -  ·         46200  ·            2  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  removeLiquidity             ·      51304  ·     174555  ·         96347  ·           41  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  swapBaseTokenForQuoteToken  ·      84821  ·     101921  ·        100606  ·           13  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Exchange                ·  swapQuoteTokenForBaseToken  ·      73704  ·     103716  ·         96873  ·           24  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ExchangeFactory         ·  createNewExchange           ·    2561955  ·    2562039  ·       2561972  ·            5  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  ExchangeFactory         ·  setFeeAddress               ·          -  ·          -  ·         30276  ·            3  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  FeeOnTransferMock       ·  approve                     ·          -  ·          -  ·         46196  ·            1  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  FeeOnTransferMock       ·  transfer                    ·          -  ·          -  ·         59045  ·            1  ·          -  │
···························|······························|·············|·············|················|···············|··············
|  Deployments                                            ·                                            ·  % of limit   ·             │
··························································|·············|·············|················|···············|··············
|  ElasticMock                                            ·          -  ·          -  ·       1130216  ·        9.1 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  ERC20PresetFixedSupply                                 ·          -  ·          -  ·        924077  ·        7.4 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  Exchange                                               ·          -  ·          -  ·       2686343  ·       21.6 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  ExchangeFactory                                        ·          -  ·          -  ·       3518140  ·       28.3 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  FeeOnTransferMock                                      ·          -  ·          -  ·       1186152  ·        9.5 %  ·          -  │
··························································|·············|·············|················|···············|··············
|  MathLib                                                ·          -  ·          -  ·       1408580  ·       11.3 %  ·          -  │
·---------------------------------------------------------|-------------|-------------|----------------|---------------|-------------·

GalloDaSballo commented 2 years ago

I still believe there's merit to this finding: By having the storage pointer for the write operations While having memory cached values for reads Reducing the amount of SLOAD will save 100 gas each time So I think there has to be room for improvement.

That said, the sponsor implemented the advice given here (not the advice we can imagine), and we can't argue with the numbers.

Marking as invalid, but recommend the sponsor to consider separating storage for writes and memory for reads and am pretty confident there is gas to be saved there

code-423n4 / 2022-01-elasticswap-findings

Remove unnecessary storage parameter can make the code simpler and save gas #154

Handle

Vulnerability details