ElasticSwap contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings.

Proof of Concept

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

src\contracts\Exchange.sol:137:                    "Exchange: FEE_ON_TRANSFER_NOT_SUPPORTED"
src\contracts\Exchange.sol:179:            "Exchange: MINS_MUST_BE_GREATER_THAN_ZERO"
src\contracts\Exchange.sol:207:            "Exchange: INSUFFICIENT_BASE_QTY"
src\contracts\Exchange.sol:212:            "Exchange: INSUFFICIENT_QUOTE_QTY"
src\contracts\Exchange.sol:268:            "Exchange: INSUFFICIENT_TOKEN_QTY"
src\contracts\Exchange.sol:305:            "Exchange: INSUFFICIENT_TOKEN_QTY"
src\contracts\ExchangeFactory.sol:47:            "ExchangeFactory: INVALID_TOKEN_ADDRESS"
src\contracts\ExchangeFactory.sol:52:            "ExchangeFactory: DUPLICATE_EXCHANGE"
src\contracts\ExchangeFactory.sol:75:            "ExchangeFactory: INVAlID_FEE_ADDRESS"
src\libraries\MathLib.sol:128:            "MathLib: INSUFFICIENT_LIQUIDITY"
src\libraries\MathLib.sol:250:            "MathLib: INSUFFICIENT_DECAY"
src\libraries\MathLib.sol:267:            "MathLib: INSUFFICIENT_CHANGE_IN_DECAY"
src\libraries\MathLib.sol:308:            "MathLib: INSUFFICIENT_DECAY"
src\libraries\MathLib.sol:337:            "MathLib: INSUFFICIENT_CHANGE_IN_DECAY"
src\libraries\MathLib.sol:470:                    "MathLib: INSUFFICIENT_BASE_QTY"
src\libraries\MathLib.sol:475:                    "MathLib: INSUFFICIENT_QUOTE_QTY"
src\libraries\MathLib.sol:497:                "MathLib: INSUFFICIENT_BASE_QTY_DESIRED"
src\libraries\MathLib.sol:501:                "MathLib: INSUFFICIENT_QUOTE_QTY_DESIRED"
src\libraries\MathLib.sol:557:                "MathLib: INSUFFICIENT_QUOTE_QTY"
src\libraries\MathLib.sol:572:                "MathLib: INSUFFICIENT_BASE_QTY"
src\libraries\MathLib.sol:608:            "MathLib: INSUFFICIENT_BASE_TOKEN_QTY"
src\libraries\MathLib.sol:641:            "MathLib: INSUFFICIENT_BASE_TOKEN_QTY"
src\libraries\MathLib.sol:665:            "MathLib: INSUFFICIENT_TOKEN_QTY"
src\libraries\MathLib.sol:677:            "MathLib: INSUFFICIENT_QUOTE_TOKEN_QTY"

Tools Used

VS Code

Recommended Mitigation Steps

Replace revert strings with custom errors.

Handle

Dravee

Vulnerability details

Impact

Avoiding an unnecessary storage reading / function call.

Proof of Concept

In Exchange.sol:removeLiquidity(): the code is as such (see @audit-info comments):

File: Exchange.sol
168:     function removeLiquidity(
169:         uint256 _liquidityTokenQty,
170:         uint256 _baseTokenQtyMin,
171:         uint256 _quoteTokenQtyMin,
172:         address _tokenRecipient,
173:         uint256 _expirationTimestamp
174:     ) external nonReentrant() {
175:         isNotExpired(_expirationTimestamp);
176:         require(this.totalSupply() > 0, "Exchange: INSUFFICIENT_LIQUIDITY"); //@audit-info 1st this.totalSupply() call
177:         require(
178:             _baseTokenQtyMin > 0 && _quoteTokenQtyMin > 0,
179:             "Exchange: MINS_MUST_BE_GREATER_THAN_ZERO"
180:         );
181: 
182:         uint256 baseTokenReserveQty =
183:             IERC20(baseToken).balanceOf(address(this));
184:         uint256 quoteTokenReserveQty =
185:             IERC20(quoteToken).balanceOf(address(this));
186: 
187:         uint256 totalSupplyOfLiquidityTokens = this.totalSupply(); //@audit-info 2nd this.totalSupply() call

As we can see, this.totalSupply(), which fetches the storage variable _totalSupply from ERC20 is called twice. By moving totalSupplyOfLiquidityTokens earlier and using it twice, it's possible to replace a function call + SLOAD with an MLOAD:

File: Exchange.sol
168:     function removeLiquidity(
169:         uint256 _liquidityTokenQty,
170:         uint256 _baseTokenQtyMin,
171:         uint256 _quoteTokenQtyMin,
172:         address _tokenRecipient,
173:         uint256 _expirationTimestamp
174:     ) external nonReentrant() {
175:         isNotExpired(_expirationTimestamp);
176:         uint256 totalSupplyOfLiquidityTokens = this.totalSupply(); //@audit-info This moved up
177:         require(totalSupplyOfLiquidityTokens > 0, "Exchange: INSUFFICIENT_LIQUIDITY"); //@audit-info totalSupplyOfLiquidityTokens call instead of this.totalSupply()
178:         require(
179:             _baseTokenQtyMin > 0 && _quoteTokenQtyMin > 0,
180:             "Exchange: MINS_MUST_BE_GREATER_THAN_ZERO"
181:         );
182: 
183:         uint256 baseTokenReserveQty =
184:             IERC20(baseToken).balanceOf(address(this));
185:         uint256 quoteTokenReserveQty =
186:             IERC20(quoteToken).balanceOf(address(this));
187: 
188:         // calculate any DAO fees here.
189:         uint256 liquidityTokenFeeQty =
190:             MathLib.calculateLiquidityTokenFees(
191:                 totalSupplyOfLiquidityTokens,
192:                 internalBalances
193:             );
194: 
195:         // we need to factor this quantity in to any total supply before redemption
196:         totalSupplyOfLiquidityTokens += liquidityTokenFeeQty;

Tools Used

VS Code

Recommended Mitigation Steps

Move the caching of this.totalSupply() earlier to save gas

Handle

Dravee

Vulnerability details

Impact

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

Proof of Concept

Here, the code is as follows:

File: MathLib.sol
253:         if (_quoteTokenQtyDesired > maxQuoteTokenQty) {
254:             quoteTokenQty = maxQuoteTokenQty;
255:         } else {
256:             quoteTokenQty = _quoteTokenQtyDesired;
257:         }

This can be optimized as such:

File: MathLib.sol
253:         if (_quoteTokenQtyDesired >= maxQuoteTokenQty) {
254:             quoteTokenQty = maxQuoteTokenQty;
255:         } else {
256:             quoteTokenQty = _quoteTokenQtyDesired;
257:         }

Tools Used

VS Code

Recommended Mitigation Steps

Use >= instead of > to avoid some opcodes

Handle

Dravee

Vulnerability details

This is following a similar submission, but with another line of code that can safely be wrapped in an unchecked block.

Impact

Increased gas cost due to unnecessary automatic overflow/underflow checks.

Proof of Concept

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers.

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

This can't get underflowed:

File: Exchange.sol
228:             internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;

This is due to the conditional flow, as the only way to execute this line is if quoteTokenQtyToReturn <= internalBalances.quoteTokenReserveQty:

File: Exchange.sol
225:         if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) {
226:             internalBalances.quoteTokenReserveQty = 0;
227:         } else {
228:             internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;
229:         }

Tools Used

VS Code

Recommended Mitigation Steps

Uncheck arithmetic operations when the risk of underflow or overflow is already contained.

Handle

Dravee

Vulnerability details

Impact

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

POC

Instances include:

libraries\MathLib.sol:43:        return ((a * WAD) + (b / 2)) / b;
libraries\MathLib.sol:55:        return ((a + (n / 2)) / n) * n;
libraries\MathLib.sol:67:        return ((a * b) + (WAD / 2)) / WAD;
libraries\MathLib.sol:85:            uint256 x = y / 2 + 1;
libraries\MathLib.sol:88:                x = (y / x + x) / 2;

Tools Used

VS Code

Recommended Mitigation Steps

Replace / 2 with >> 1

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive (~100 gas) compared to MLOADs (~3 gas). Minimizing them can save gas.

Proof of Concept

In Exchange.sol:removeLiquidity(): the code is as such (see @audit-info comments for the SLOADs and SSTOREs):

File: Exchange.sol
188:         // calculate any DAO fees here.
189:         uint256 liquidityTokenFeeQty =
190:             MathLib.calculateLiquidityTokenFees(
191:                 totalSupplyOfLiquidityTokens,
192:                 internalBalances //@audit-info 3 SLOADs
193:             );
194: 
195:         // we need to factor this quantity in to any total supply before redemption
196:         totalSupplyOfLiquidityTokens += liquidityTokenFeeQty;
197: 
198:         uint256 baseTokenQtyToReturn =
199:             (_liquidityTokenQty * baseTokenReserveQty) /
200:                 totalSupplyOfLiquidityTokens;
201:         uint256 quoteTokenQtyToReturn =
202:             (_liquidityTokenQty * quoteTokenReserveQty) /
203:                 totalSupplyOfLiquidityTokens;
204: 
205:         require(
206:             baseTokenQtyToReturn >= _baseTokenQtyMin,
207:             "Exchange: INSUFFICIENT_BASE_QTY"
208:         );
209: 
210:         require(
211:             quoteTokenQtyToReturn >= _quoteTokenQtyMin,
212:             "Exchange: INSUFFICIENT_QUOTE_QTY"
213:         );
214: 
215:         // this ensure that we are removing the equivalent amount of decay
216:         // when this person exits.
217:         uint256 baseTokenQtyToRemoveFromInternalAccounting =
218:             (_liquidityTokenQty * internalBalances.baseTokenReserveQty) / //@audit-info SLOAD
219:                 totalSupplyOfLiquidityTokens;
220: 
221:         internalBalances
222:             .baseTokenReserveQty -= baseTokenQtyToRemoveFromInternalAccounting; //@audit-info SLOAD + SSTORE
223: 
224:         // We should ensure no possible overflow here.
225:         if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) { //@audit-info SLOAD
226:             internalBalances.quoteTokenReserveQty = 0;
227:         } else {
228:             internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn; //@audit-info SLOAD + SSTORE
229:         }
230: 
231:         internalBalances.kLast =
232:             internalBalances.baseTokenReserveQty *
233:             internalBalances.quoteTokenReserveQty; //@audit-info 2 SLOAD + 1 STORE
234: 

The code can be optimized to minimize the number of SLOADs. A copy of a struct from storage to memory will make as many SLOADs and MSTOREs as there are elements in it. Here, MathLib.InternalBalances's copy would take 3 SLOADs:

    struct InternalBalances {
        uint256 baseTokenReserveQty;
        uint256 quoteTokenReserveQty;
        uint256 kLast;
    }

As there are more than 3 SLOADs for internalBalances in removeLiquidity(), the struct should be cached:

File: Exchange.sol
188:         MathLib.InternalBalances memory _internalBalances = internalBalances; //@audit-info 3 SLOADs + 3 MSTOREs
189:         // calculate any DAO fees here.
190:         uint256 liquidityTokenFeeQty =
191:             MathLib.calculateLiquidityTokenFees(
192:                 totalSupplyOfLiquidityTokens,
193:                 _internalBalances //@audit-info 3 MLOAD
194:             );
195: 
196:         // we need to factor this quantity in to any total supply before redemption
197:         totalSupplyOfLiquidityTokens += liquidityTokenFeeQty;
198: 
199:         uint256 baseTokenQtyToReturn =
200:             (_liquidityTokenQty * baseTokenReserveQty) /
201:                 totalSupplyOfLiquidityTokens;
202:         uint256 quoteTokenQtyToReturn =
203:             (_liquidityTokenQty * quoteTokenReserveQty) /
204:                 totalSupplyOfLiquidityTokens;
205: 
206:         require(
207:             baseTokenQtyToReturn >= _baseTokenQtyMin,
208:             "Exchange: INSUFFICIENT_BASE_QTY"
209:         );
210: 
211:         require(
212:             quoteTokenQtyToReturn >= _quoteTokenQtyMin,
213:             "Exchange: INSUFFICIENT_QUOTE_QTY"
214:         );
215: 
216:         // this ensure that we are removing the equivalent amount of decay
217:         // when this person exits.
218:         uint256 baseTokenQtyToRemoveFromInternalAccounting =
219:             (_liquidityTokenQty * _internalBalances.baseTokenReserveQty) / //@audit-info 1 MLOAD
220:                 totalSupplyOfLiquidityTokens;
221: 
222:         _internalBalances
223:             .baseTokenReserveQty -= baseTokenQtyToRemoveFromInternalAccounting; //@audit-info 1 MLOAD + 1 MSTORE
224: 
225:         // We should ensure no possible overflow here.
226:         if (quoteTokenQtyToReturn > _internalBalances.quoteTokenReserveQty) { //@audit-info 1 MLOAD
227:             _internalBalances.quoteTokenReserveQty = 0;
228:         } else {
229:             _internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn; //@audit-info 1 MLOAD + 1 MSTORE
230:         }
231: 
232:         internalBalances.quoteTokenReserveQty = _internalBalances.quoteTokenReserveQty; //@audit-info 1 MLOAD + 1 SSTORE
233:         internalBalances.baseTokenReserveQty = _internalBalances.baseTokenReserveQty; //@audit-info 1 MLOAD + 1 SSTORE
234:         internalBalances.kLast =
235:             _internalBalances.baseTokenReserveQty *
236:             _internalBalances.quoteTokenReserveQty; //@audit-info 2 MLOAD + 1 SSTORE

Tools Used

VS Code

Recommended Mitigation Steps

Cache internalBalances in memory for the code logic in Exchange.sol:removeLiquidity() and write in storage at the end.

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive (~100 gas) compared to MLOADs (~3 gas). Minimizing them can save gas.

Proof of Concept

In MathLib.sol:calculateQuoteTokenQty(): the code is as such (see @audit-info comments for the opcodes):

File: MathLib.sol
657:     function calculateQuoteTokenQty(
658:         uint256 _baseTokenQty,
659:         uint256 _quoteTokenQtyMin,
660:         uint256 _liquidityFeeInBasisPoints,
661:         InternalBalances storage _internalBalances
662:     ) public returns (uint256 quoteTokenQty) {
663:         require(
664:             _baseTokenQty > 0 && _quoteTokenQtyMin > 0,
665:             "MathLib: INSUFFICIENT_TOKEN_QTY"
666:         );
667: 
668:         quoteTokenQty = calculateQtyToReturnAfterFees(
669:             _baseTokenQty,
670:             _internalBalances.baseTokenReserveQty,//@audit-info SLOAD
671:             _internalBalances.quoteTokenReserveQty,//@audit-info SLOAD
672:             _liquidityFeeInBasisPoints
673:         );
674: 
675:         require(
676:             quoteTokenQty > _quoteTokenQtyMin,
677:             "MathLib: INSUFFICIENT_QUOTE_TOKEN_QTY"
678:         );
679: 
680:         _internalBalances.baseTokenReserveQty += _baseTokenQty; //@audit-info SLOAD + SSTORE
681:         _internalBalances.quoteTokenReserveQty -= quoteTokenQty;//@audit-info SLOAD + SSTORE
682:     }

The code can be optimized by minimizing the number of SLOADs. Here's what I suggest:

File: MathLib.sol
657:     function calculateQuoteTokenQty(
658:         uint256 _baseTokenQty,
659:         uint256 _quoteTokenQtyMin,
660:         uint256 _liquidityFeeInBasisPoints,
661:         InternalBalances storage _internalBalances
662:     ) public returns (uint256 quoteTokenQty) {
663:         require(
664:             _baseTokenQty > 0 && _quoteTokenQtyMin > 0,
665:             "MathLib: INSUFFICIENT_TOKEN_QTY"
666:         );
667:         uint256 _internalBalancesBaseTokenReserveQty = _internalBalances.baseTokenReserveQty; //@audit-info SLOAD + MSTORE
668:         uint256 _internalBalancesQuoteTokenReserveQty = _internalBalances.quoteTokenReserveQty; //@audit-info SLOAD + MSTORE
669:         quoteTokenQty = calculateQtyToReturnAfterFees(
670:             _baseTokenQty,
671:             _internalBalancesBaseTokenReserveQty,//@audit-info MLOAD
672:             _internalBalancesQuoteTokenReserveQty,//@audit-info MLOAD
673:             _liquidityFeeInBasisPoints
674:         );
675: 
676:         require(
677:             quoteTokenQty > _quoteTokenQtyMin,
678:             "MathLib: INSUFFICIENT_QUOTE_TOKEN_QTY"
679:         );
680:         _internalBalancesBaseTokenReserveQty += _baseTokenQty; //@audit-info MLOAD + MSTORE
681:         _internalBalancesQuoteTokenReserveQty -= quoteTokenQty;//@audit-info MLOAD + MSTORE
682:         _internalBalances.baseTokenReserveQty = _baseTokenQty; //@audit-info MLOAD + SSTORE (no SLOAD)
683:         _internalBalances.quoteTokenReserveQty = quoteTokenQty;//@audit-info MLOAD + SSTORE (no SLOAD)
684:     }

Tools Used

VS Code

Recommended Mitigation Steps

Apply the suggested optimization

Handle

Dravee

Vulnerability details

Impact

It's possible to save gas by optimizing conditional flows to avoid some unnecessary opcodes

Proof of Concept

In Exchange.sol:removeLiquidity(), the code is as follows:

File: Exchange.sol
225:         if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) {
226:             internalBalances.quoteTokenReserveQty = 0;
227:         } else {
228:             internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;
229:         }

However, this can be optimized :

• Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)
• In this case here, if quoteTokenQtyToReturn == internalBalances.quoteTokenReserveQty: internalBalances.quoteTokenReserveQty = 0 should be used
• Avoiding the else clause would avoid some opcodes (1 SUB = 3 gas, 2 MLOADs = 6 gas...)

The code would become:

File: Exchange.sol
225:         if (quoteTokenQtyToReturn >= internalBalances.quoteTokenReserveQty) {
226:             internalBalances.quoteTokenReserveQty = 0;
227:         } else {
228:             internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;
229:         }

Tools Used

VS Code

Recommended Mitigation Steps

Use the non-strict greater-than operator in this particular case

Handle

Dravee

Vulnerability details

Impact

Saving the gas cost from the calculation

Proof of Concept

See the @audit-info tags:

File: MathLib.sol
141:     function calculateQtyToReturnAfterFees(
142:         uint256 _tokenASwapQty,
143:         uint256 _tokenAReserveQty,
144:         uint256 _tokenBReserveQty,
145:         uint256 _liquidityFeeInBasisPoints
146:     ) public pure returns (uint256 qtyToReturn) {
147:         uint256 tokenASwapQtyLessFee = //@audit-info == 0 if _tokenASwapQty == 0
148:             _tokenASwapQty * (BASIS_POINTS - _liquidityFeeInBasisPoints); 
149:         qtyToReturn =
150:             (tokenASwapQtyLessFee * _tokenBReserveQty) / //@audit-info 0 is possible if _tokenBReserveQty == 0 or above is equal to 0
151:             ((_tokenAReserveQty * BASIS_POINTS) + tokenASwapQtyLessFee);
152:     }

Tools Used

VS Code

Recommended Mitigation Steps

Return 0 if _tokenASwapQty == 0 || _tokenBReserveQty == 0 or the && equivalent. Here's an example:

File: MathLib.sol
141:     function calculateQtyToReturnAfterFees(
142:         uint256 _tokenASwapQty,
143:         uint256 _tokenAReserveQty,
144:         uint256 _tokenBReserveQty,
145:         uint256 _liquidityFeeInBasisPoints
146:     ) public pure returns (uint256 qtyToReturn) {
147:         if(_tokenASwapQty != 0 && _tokenBReserveQty != 0){
148:             uint256 tokenASwapQtyLessFee = _tokenASwapQty * 
149:                 (BASIS_POINTS - _liquidityFeeInBasisPoints); 
150:             qtyToReturn = (tokenASwapQtyLessFee * _tokenBReserveQty) / 
151:                 ((_tokenAReserveQty * BASIS_POINTS) + tokenASwapQtyLessFee);
152:         }
153:     }

(here qtyToReturn if set to 0 by default so the value returned would be 0)

Handle

Dravee

Vulnerability details

Impact

Some of the require statements can be placed earlier to reduce gas usage on revert.

Proof of Concept

The following can be reordered to save gas on revert:

File: MathLib.sol
464:                 tokenQtys.baseTokenQty += baseTokenQtyFromDecay;
465:                 tokenQtys.quoteTokenQty += quoteTokenQtyFromDecay;
466:                 tokenQtys.liquidityTokenQty += liquidityTokenQtyFromDecay;
467: 
468:                 require(
469:                     tokenQtys.baseTokenQty >= _baseTokenQtyMin,
470:                     "MathLib: INSUFFICIENT_BASE_QTY"
471:                 );
472: 
473:                 require(
474:                     tokenQtys.quoteTokenQty >= _quoteTokenQtyMin,
475:                     "MathLib: INSUFFICIENT_QUOTE_QTY"
476:                 );

to

File: MathLib.sol
464:                 tokenQtys.baseTokenQty += baseTokenQtyFromDecay;
465:                 
466:                 require(
467:                     tokenQtys.baseTokenQty >= _baseTokenQtyMin,
468:                     "MathLib: INSUFFICIENT_BASE_QTY"
469:                 );
470: 
471:                 tokenQtys.quoteTokenQty += quoteTokenQtyFromDecay;
472: 
473:                 require(
474:                     tokenQtys.quoteTokenQty >= _quoteTokenQtyMin,
475:                     "MathLib: INSUFFICIENT_QUOTE_QTY"
476:                 );
477: 
478:                 tokenQtys.liquidityTokenQty += liquidityTokenQtyFromDecay;

Tools Used

VS Code

Recommended Mitigation Steps

Relocate the said require statements

Handle

Dravee

Vulnerability details

Impact

Some of the require statements can be placed earlier to reduce gas usage on revert.

Proof of Concept

The following can be reordered to save gas on revert:

File: Exchange.sol
198:         uint256 baseTokenQtyToReturn =
199:             (_liquidityTokenQty * baseTokenReserveQty) /
200:                 totalSupplyOfLiquidityTokens;
201:         uint256 quoteTokenQtyToReturn =
202:             (_liquidityTokenQty * quoteTokenReserveQty) /
203:                 totalSupplyOfLiquidityTokens;
204: 
205:         require(
206:             baseTokenQtyToReturn >= _baseTokenQtyMin,
207:             "Exchange: INSUFFICIENT_BASE_QTY"
208:         );
209: 
210:         require(
211:             quoteTokenQtyToReturn >= _quoteTokenQtyMin,
212:             "Exchange: INSUFFICIENT_QUOTE_QTY"
213:         );

to

File: Exchange.sol
198:         uint256 baseTokenQtyToReturn =
199:             (_liquidityTokenQty * baseTokenReserveQty) /
200:                 totalSupplyOfLiquidityTokens;
201: 
202:         require(
203:             baseTokenQtyToReturn >= _baseTokenQtyMin,
204:             "Exchange: INSUFFICIENT_BASE_QTY"
205:         );
206: 
207:         uint256 quoteTokenQtyToReturn =
208:             (_liquidityTokenQty * quoteTokenReserveQty) /
209:                 totalSupplyOfLiquidityTokens;
210: 
211:         require(
212:             quoteTokenQtyToReturn >= _quoteTokenQtyMin,
213:             "Exchange: INSUFFICIENT_QUOTE_QTY"
214:         );

Tools Used

VS Code

Recommended Mitigation Steps

Relocate the said require statements

Handle

Dravee

Vulnerability details

Impact

Functions marked as payable are 24 gas cheaper than their counterpart (in non-payable functions, Solidity adds an extra check to ensure msg.value is zero). When users can't mistakenly send ETH to a function (as an example, when there's an onlyOwner modifier or alike), it is safe to mark it as payable.

Proof of Concept

https://github.com/code-423n4/2022-01-elasticswap/blob/main/elasticswap/src/contracts/ExchangeFactory.sol#L72 :

File: ExchangeFactory.sol
72:     function setFeeAddress(address _feeAddress) external onlyOwner { //@audit-info : modifier == onlyOwner and visibility == external => this function should be made payable to save gas

Tools Used

VS Code

Recommended Mitigation Steps

Mark as payable the external functions that have a trusted modifier which prevents users from mistakenly sending Ether.

Handle

Dravee

Vulnerability details

Impact

Using both named returns and a return statement isn't necessary. It also increases gas cost and decrease code clarity.

Proof of Concept

The following functions use named returns but contain a return statement too:

File: MathLib.sol
227:     function calculateAddQuoteTokenLiquidityQuantities(
228:         uint256 _quoteTokenQtyDesired,
229:         uint256 _quoteTokenQtyMin,
230:         uint256 _baseTokenReserveQty,
231:         uint256 _totalSupplyOfLiquidityTokens,
232:         InternalBalances storage _internalBalances
233:     ) public returns (uint256 quoteTokenQty, uint256 liquidityTokenQty) {
...
282:         return (quoteTokenQty, liquidityTokenQty);


File: MathLib.sol
297:     function calculateAddBaseTokenLiquidityQuantities(
298:         uint256 _baseTokenQtyDesired,
299:         uint256 _baseTokenQtyMin,
300:         uint256 _baseTokenReserveQty,
301:         uint256 _totalSupplyOfLiquidityTokens,
302:         InternalBalances memory _internalBalances
303:     ) public pure returns (uint256 baseTokenQty, uint256 liquidityTokenQty) {
...
362:         return (baseTokenQty, liquidityTokenQty);

Tools Used

VS Code

Recommended Mitigation Steps

Remove the redundant return statements L282 and L362 on MathLib.sol