ElasticSwap contest - ye0lde's results

Handle

ye0lde

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

It's clear the sponsor is committed to consistent, verbose revert messages. But I just wanted to note that a lot of these messages are close to the limit and could be shortened and be under the limit just by abbreviating the "contract" name or removing a space.

Proof of Concept

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/Exchange.sol#L137 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/Exchange.sol#L176

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/ExchangeFactory.sol#L44 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/ExchangeFactory.sol#L47 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/ExchangeFactory.sol#L52 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/ExchangeFactory.sol#L75

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L267 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L337 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L497 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L501 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L608 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L641

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Consider shortening the revert strings to fit in 32 bytes or using custom errors in the future.

Handle

ye0lde

Vulnerability details

Impact

A struct is being assigned its default value which is unnecessary. Removing the assignment will save gas when deploying.

Proof of Concept

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/Exchange.sol#L27-L28

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Remove the assignment.

Or if you feel it is important to show the default assignment will occur then replace the assignments with a comment.

I suggest the following:

Change this:

    MathLib.InternalBalances public internalBalances =   
        MathLib.InternalBalances(0, 0, 0);

to something like:

    MathLib.InternalBalances public internalBalances;      // Tracking x*y=k with x=0, y=0, k=0 initially

Handle

ye0lde

Vulnerability details

Impact

Redundant arithmetic underflow/overflow checks can be avoided when an underflow/overflow cannot happen.

Proof of Concept

The "unchecked" keyword can be applied to these since there is an if statement before to ensure the arithmetic operations would not cause an integer underflow or overflow.

https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/contracts/Exchange.sol#L224-L229

Change the code at 224 to:

        unchecked {
            // We should ensure no possible overflow here.           
            if (quoteTokenQtyToReturn > internalBalances.quoteTokenReserveQty) {
                internalBalances.quoteTokenReserveQty = 0;
            } else {
                internalBalances.quoteTokenReserveQty -= quoteTokenQtyToReturn;
            }
        }

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

Change the code at 75 to:

        unchecked {
            if (a >= b) {
                return a - b;
            }
            return b - a;
        }

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Add the "unchecked" keyword as shown above.

Handle

ye0lde

Vulnerability details

Impact

Using the existing named returns more efficiently can reduce gas usage and improve code clarity.

Proof of Concept

These functions have named return variables and don't need explicit return statements at the end.

I suggest deleting the following explicit returns: https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L282 https://github.com/code-423n4/2022-01-elasticswap/blob/d107a198c0d10fbe254d69ffe5be3e40894ff078/elasticswap/src/libraries/MathLib.sol#L362

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

See POC