Timeswap contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2023-01-timeswap/blob/main/packages/v2-option/src/TimeswapV2Option.sol#L145-L148 https://github.com/code-423n4/2023-01-timeswap/blob/main/packages/v2-option/src/TimeswapV2Option.sol#L235

Vulnerability details

Impact

According to Whitepaper 1.1 Permissionless:

"In Timeswap, liquidity providers can create pools for any ERC20 pair, without permission. It is designed to be generalized and works for any pair of tokens, at any time frame, and at any market state ...

If fee on transfer token(s) is/are entailed, it will specifically make mint() and swap() revert in TimeswapV2Option.sol when checking if the token0 or token1 balance target is achieved.

Proof of Concept

File: TimeswapV2Option.sol#L144-L148

        // check if the token0 balance target is achieved.
        if (token0AndLong0Amount != 0) Error.checkEnough(IERC20(token0).balanceOf(address(this)), currentProcess.balance0Target);

        // check if the token1 balance target is achieved.
        if (token1AndLong1Amount != 0) Error.checkEnough(IERC20(token1).balanceOf(address(this)), currentProcess.balance1Target);

File: TimeswapV2Option.sol#L234-L235

        // check if the token0 or token1 balance target is achieved.
        Error.checkEnough(IERC20(param.isLong0ToLong1 ? token1 : token0).balanceOf(address(this)), param.isLong0ToLong1 ? currentProcess.balance1Target : currentProcess.balance0Target);

File: Error.sol#L148-L153

    /// @dev Reverts when token amount not received.
    /// @param balance The balance amount being subtracted.
    /// @param balanceTarget The amount target.
    function checkEnough(uint256 balance, uint256 balanceTarget) internal pure {
        if (balance < balanceTarget) revert NotEnoughReceived(balance, balanceTarget);
    }

As can be seen from the code blocks above, checkEnough() is meant to be reverting when token amount has not been received. But in the case of deflationary tokens, the error is going to be thrown even though the token amount has been received due to the fee factor making balance < balanceTarget, i.e the contract balance of token0 and/or token1 always less than currentProcess.balance0Target or currentProcess.balance1Target.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider:

1. whitelisting token0 and token1 ensuring no fee-on-transfer token is allowed when deploying a new Timeswap V2 Option pair contract, or
2. calculating the balance before and after the transfer to the recipient during the process, and use the difference between those two balances as the amount received rather than using the input amount (token0AndLong0Amount or token1AndLong1Amount) if deflationary token is going to be allowed in the protocol.

Typo/grammatical mistakes

File: FullMath.sol#L1

Note: The following correction represents only one of the numerous contract instances with the same grammatical error entailed.

- //SPDX-License-Identifier: Unlicense
+ //SPDX-License-Identifier: Unlicensed

File: Math.sol#L84

-    /// @dev Gets the min of two uint256 number.
+    /// @dev Gets the min of two uint256 numbers.

File: ITimeswapV2PoolBurnCallback.sol#L8 File: ITimeswapV2PoolLeverageCallback.sol#L8 File: ITimeswapV2PoolMintCallback.sol#L8 File: ITimeswapV2PoolDeleverageCallback.sol#L8

-    /// @dev Returns the amount of long0 position and long1 positions chosen to be withdrawn.
+    /// @dev Returns the amount of long0 position and long1 position chosen to be withdrawn.

File: ITimeswapV2PoolRebalanceCallback.sol#L10 File: ITimeswapV2PoolLeverageCallback.sol#L10 File: ITimeswapV2PoolMintCallback.sol#L10 File: ITimeswapV2PoolDeleverageCallback.sol#L10 File: Param.sol#L11-L13 File: Pool.sol#L168

-    /// @dev The long0 positions or long1 positions will already be minted to the receipient.
+    /// @dev The long0 positions or long1 positions will already be minted to the recipient.

File: OwnableTwoSteps.sol#L9

- /// @dev contract for ownable implementation with a safety two step owner transfership.
+ /// @dev contract for ownable implementation with a safety two step ownership transfer.

File: PoolFactory.sol#L41-L42

-    /// @return optionPair The retreived option pair address.
-    /// @return poolPair The retreived pool pair address.
+    /// @return optionPair The retrieved option pair address.
+    /// @return poolPair The retrieved pool pair address.

File: ReentrancyGuard.sol#L11

-    /// @dev The initial state which must be change to NOT_ENTERED when first interacting.
+    /// @dev The initial state which must be changed to NOT_ENTERED when first interacting.

File: StrikeConversion.sol#L22

-    /// @param amount The amount ot be converted. Token0 amount when zeroToOne. Token1 amount when oneToZero.
+    /// @param amount The amount to be converted. Token0 amount when zeroToOne. Token1 amount when oneToZero.

File: TimeswapV2Option.sol#L47

-    /// @dev mapping of all option state for all strikes and maturies.
+    /// @dev mapping of all option state for all strikes and maturities.

File: ITimeswapV2Pool.sol#L132

-    /// @param to If isLong0ToLong1 then receipient of long0 positions, ekse recipient of long1 positions.
+    /// @param to If isLong0ToLong1 then recipient of long0 positions, else recipient of long1 positions.

Minimization of truncation

The number of divisions in an equation should be reduced to minimize truncation frequency, serving to achieve higher precision. And, where deemed fit, comment the code line with the original multiple division arithmetic operation for clarity reason.

For instance, the code line below involving two divisions may be refactored as follows:

File: Math.sol#L77

-                estimate = (value / estimate + estimate) >> 1;
+                estimate = (value + estimate * estimate) / (estimate * 2);

Lines too long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here are some of the instances entailed:

File: StrikeConversion.sol#L27 File: TimeswapV2Pool.sol#L31-L34 File: TimeswapV2Pool.sol#L293-L297

Inappropriate error thrown

The second if block of create() in TimeswapV2PoolFactory.sol throws Error.zeroAddress() if pair != address(0), which is misleading.

Consider having the affected code line refactored as follows to avoid any confusion to the users/developers:

Note: The suggested error may have to be added/implemented in Error.sol.

File: TimeswapV2PoolFactory.sol#L59-L70

    function create(address optionPair) external override returns (address pair) {
        if (optionPair == address(0)) Error.zeroAddress();

        pair = pairs[optionPair];
-        if (pair != address(0)) Error.zeroAddress();
+        if (pair != address(0)) Error.alreadyCreatedPool();

        pair = deploy(address(this), optionPair, transactionFee, protocolFee);

        pairs[optionPair] = pair;

        emit Create(msg.sender, optionPair, pair);
    }

Solidity's Style Guide on contract layout

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

For instance, it will be of added values to the users and developers if:

• @ return is also provided on the functions of ConstantSum.sol.
• at least a minimalist NatSpec is provided on FeesPosition.sol.

Inappropriate comment

The return code line in the function below entails only a simple addition that is devoid of division. The comment, truncation is desired is therefore deemed non-matching. Consider removing/editing the comment where possible:

File: TimeswapV2Pool.sol#L82-L84

    function blockTimestamp(uint96 durationForward) internal view virtual returns (uint96) {
        return uint96(block.timestamp + durationForward); // truncation is desired
    }

Erroneous function NatSpec

File: ITimeswapV2PoolDeployer.sol#L11-L18

    /// @notice Get the parameters to be used in constructing the pair, set transiently during pair creation.
    /// @dev Called by the pair constructor to fetch the parameters of the pair.
    /// @return poolFactory The poolFactory address.
-    /// @param optionPair The Timeswap V2 OptionPair address.
+    /// @return optionPair The Timeswap V2 OptionPair address.
-    /// @param transactionFee The transaction fee earned by the liquidity providers.
+    /// @return transactionFee The transaction fee earned by the liquidity providers.
-    /// @param protocolFee The protocol fee earned by the DAO.
+    /// @return protocolFee The protocol fee earned by the DAO.
    function parameter() external view returns (address poolFactory, address optionPair, uint256 transactionFee, uint256 protocolFee);
}

File: ITimeswapV2PoolFactory.sol#L37-L42

    /// @dev Creates a Timeswap V2 Pool based on option parameter.
    /// @dev Cannot create a duplicate Timeswap V2 Pool with the same option parameter.
    /// @param option The address of the option contract used by the pool.
-    /// @param poolPair The address of the Timeswap V2 Pool contract created.
+    /// @return poolPair The address of the Timeswap V2 Pool contract created.
    function create(address option) external returns (address poolPair);
}

Missing zero value checks

When initializing the pool, a boundary and a zero value check has correspondingly been applied on maturity and rate.

A zero value check should likewise be executed on strike to make the input parameters of initialize() more error free:

(Note: Additionally, the zero value checks is suggested moving to the first line of the function logic so it could become the first line of defense to revert the earliest if need be.)

File: TimeswapV2Pool.sol#L175-L181

    function initialize(uint256 strike, uint256 maturity, uint160 rate) external override noDelegateCall {
+        if (rate == 0 || strike == 0) Error.cannotBeZero();
        if (maturity < blockTimestamp(0)) Error.alreadyMatured(maturity, blockTimestamp(0));
-        if (rate == 0) Error.cannotBeZero();
        addPoolEnumerationIfNecessary(strike, maturity);

        pools[strike][maturity].initialize(rate);
    }

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = 0 instance below may be refactored as follows:

File: Pool.sol#L685

-                    pool.long1Balance = 0;
+                    delete pool.long1Balance;

Variable Assignment in Conditional Check

Making a variable assignment in a conditional statement deviates from the standard use and intention of the check and can easily lead to confusion.

Here are the instances entailed that should have the needed assignment cached before the conditional statement as follows:

File: Pool.sol

- 681:                if ((long1Amount = param.delta) == long1AmountAdjustFees) {

+                long1Amount = param.delta);
+                if (long1Amount == long1AmountAdjustFees) {

- 702:                if ((long0Amount = param.delta) == long0AmountAdjustFees) {

+                long0Amount = param.delta);
+                if (long0Amount == long0AmountAdjustFees) {

Sort _tokenA and _takenB on create()

In create() of TimeswapV2OptionFactory.sol, instead of reverting the function if the tokens have not been sorted, consider having the function logic refactored as follows to better facilitate the function call:

File: TimeswapV2OptionFactory.sol#L43-L56

    function create(address token0, address token1) external override returns (address optionPair) {
        if (token0 == address(0)) Error.zeroAddress();
        if (token1 == address(0)) Error.zeroAddress();
-        OptionPairLibrary.checkCorrectFormat(token0, token1);
+        (token0_, token1_) = token0 < token1 ? (token0, token1) : (token1, token0);
-        optionPair = optionPairs[token0][token1];
+        optionPair = optionPairs[token0_][token1_];
-        OptionPairLibrary.checkDoesNotExist(token0, token1, optionPair);
+        OptionPairLibrary.checkDoesNotExist(token0_, token1_, optionPair);

-        optionPair = deploy(address(this), token0, token1);
+        optionPair = deploy(address(this), token0_, token1_);

-        optionPairs[token0][token1] = optionPair;
+        optionPairs[token0_][token1_] = optionPair;

-        emit Create(msg.sender, token0, token1, optionPair);
+        emit Create(msg.sender, token0_, token1_, optionPair);
    }

Comment and documentation mismatch

According to the whitepaper,

z/(x+y) is the marginal interest (I) rate per second of the Short (z) per total Long (x + y).

However, it is stated differently in the NatSpec comment below which should be corrected as follows:

File: ITimeswapV2Pool.sol#L170

-    /// @dev the square root of interest rate is z/(x+y) where z is the short amount, x+y is the long0 amount, and y is the long1 amount.
+    /// @dev the square root of interest rate is sqrt(z/(x+y)) where z is the short amount, x is the long0 amount, and y is the long1 amount.

Return statement on named returns

Functions with named returns should not have a return statement to avoid unnecessary confusion.

For instance, the following min() may be refactored as:

File: Math.sol#L88-L90

    function min(uint256 value1, uint256 value2) internal pure returns (uint256 result) {
-        return value1 < value2 ? value1 : value2;
+        result = value1 < value2 ? value1 : value2;
    }

Use a more recent version of solidity

The protocol adopts version 0.8.8 on writing contracts. For better security, it is best practice to use the latest Solidity version, 0.8.17.

Security fix list in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Redundant check

In transferLiquidity() of TimeswapV2Pool.sol, pool.liquidity != 0 will be checked in the external transferLiquidity().

As such, invoking hasLiquidity(strike, maturity) preliminarily is unnecessary although this will make the call revert earlier if pool.liquidity == 0.

(Note: A zero value check for strike and maturity is optionally included to replace hasLiquidity(), which is still going to cheaper in terns of gas savings. This is because the former is an inline code whereas the latter is a private function call.)

Consider having the function refactored as follows to save gas on contract size and all successful calls:

File: TimeswapV2Pool.sol#L152-L162

    function transferLiquidity(uint256 strike, uint256 maturity, address to, uint160 liquidityAmount) external override {
-        hasLiquidity(strike, maturity);
+        if (strike == 0 || maturity == 0) Error.cannotBeZero();

        if (blockTimestamp(0) > maturity) Error.alreadyMatured(maturity, blockTimestamp(0));
        if (to == address(0)) Error.zeroAddress();
        if (liquidityAmount == 0) Error.zeroInput();

        pools[strike][maturity].transferLiquidity(to, liquidityAmount, blockTimestamp(0));

        emit TransferLiquidity(strike, maturity, msg.sender, to, liquidityAmount);
    }

Unneeded if block

In updateDurationWeightBeforeMaturity() of Pool.sol, blockTimestamp == blockTimestamp(0) and blockTimestamp(0) == block.timestamp. As such, pool.lastTimestamp < blockTimestamp will always hold true.

Consider having the if block removed to save gas both on contract deployment and function calls:

File: Pool.sol#L128-L137

    function updateDurationWeightBeforeMaturity(Pool storage pool, uint96 blockTimestamp) private {
-        if (pool.lastTimestamp < blockTimestamp)
            (pool.lastTimestamp, pool.shortFeeGrowth) = updateDurationWeight(
                pool.liquidity,
                pool.sqrtInterestRate,
                pool.shortFeeGrowth,
                DurationCalculation.unsafeDurationFromLastTimestampToNow(pool.lastTimestamp, blockTimestamp),
                blockTimestamp
            );
    }

Unusable functions and arrays

create() in TimeswapV2PoolFactory.sol and TimeswapV2OptionFactory.sol does not have pair and optionPair respectively pushed into getByIndex. For this reason, calling numberOfPairs() is going to always return getByIndex.length == 0.

Consider removing these unusable functions and state arrays to save gas on contract size:

File: TimeswapV2PoolFactory.sol

- 33:    address[] public override getByIndex;

- 52:    function numberOfPairs() external view override returns (uint256) {
- 53:        return getByIndex.length;
- 54:    }

File: TimeswapV2OptionFactory.sol

- 25:    address[] public override getByIndex;

- 36:    function numberOfPairs() external view override returns (uint256) {
- 37:        return getByIndex.length;
- 38:    }

Inline code

Embedded function call entailing only one line of code may be made inline to save gas.

For instance, in create() of TimeswapV2OptionFactory.sol, OptionPairLibrary.checkDoesNotExist(token0, token1, optionPair) is only making sure optionPair == address(0).

For this reason, consider having the affected code line refactored as follows:

Note: The suggested error may have to be added/implemented in Error.sol.

File: TimeswapV2OptionFactory.sol#L43-L56

    function create(address token0, address token1) external override returns (address optionPair) {
        if (token0 == address(0)) Error.zeroAddress();
        if (token1 == address(0)) Error.zeroAddress();
        OptionPairLibrary.checkCorrectFormat(token0, token1);

        optionPair = optionPairs[token0][token1];
-        OptionPairLibrary.checkDoesNotExist(token0, token1, optionPair);
+        if (optionPair != address(0)) Error.alreadyExisted();

        optionPair = deploy(address(this), token0, token1);

        optionPairs[token0][token1] = optionPair;

        emit Create(msg.sender, token0, token1, optionPair);
    }

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: BytesLib.sol#L13

-        require(_length + 31 >= _length, "slice_overflow");
// Rationale for subtracting 1 on the right side of the inequality:
// x >= 10; [10, 11, 12, ...]
// x > 10 - 1 is the same as x > 9; [10, 11, 12 ...] 
+        require(_length + 31 > _length - 1, "slice_overflow");

Similarly, the <= instance below may be replaced with < as follows:

File: FullMath.sol#L189

-        if (divisor <= product1) revert MulDivOverflow(multiplicand, multiplier, divisor);
// Rationale for adding 1 on the right side of the inequality:
// x <= 10; [10, 9, 8, ...]
// x < 10 + 1 is the same as x < 11; [10, 9, 8 ...]
+        if (divisor < product1 + 1) revert MulDivOverflow(multiplicand, multiplier, divisor);

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instance below may be refactored as follows:

File: FullMath.sol#L163

-            productA1 += (quotient1 * divisor);
+            productA1 = productA1 + (quotient1 * divisor);

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the && instance below may be refactored as follows:

Note: Comment on the changes made for better readability where deemed fit.

File: Math.sol#L51

-        if (roundUp && dividend % divisor != 0) quotient++;
+        if (!(!roundUp || dividend % divisor == 0)) quotient++;

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the specific instance below may be refactored as follows:

File: ConstantProduct.sol#L149-L150

-        if (isAdd) longAmount -= fees;
-        else shortAmount -= fees;

+      isAdd
+          ? longAmount -= fees
+          : shortAmount -= fees;

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

For instance, the specific instance below may be refactored as follows:

File: Token.sol#L275

-        FeesPosition memory feesPosition = _feesPositions[id][owner];
+        FeesPosition storage feesPosition = _feesPositions[id][owner];

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas.

For instance, the code b;lock below may be refactored as follows:

File: FeesPosition.sol#L52-L56

    function mint(FeesPosition storage feesPosition, uint256 long0Fees, uint256 long1Fees, uint256 shortFees) internal {
+      unchecked {
        feesPosition.long0Fees += long0Fees;
        feesPosition.long1Fees += long1Fees;
        feesPosition.shortFees += shortFees;
+      }
    }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.13",

settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use of named returns for local variables saves gas

You can have further advantages in terms of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: TimeswapV2LiquidityToken.sol#L255-L257

-    function _additionalConditionAddTokenToOwnerEnumeration(address to, uint256 id) internal view override returns (bool) {
+    function _additionalConditionAddTokenToOwnerEnumeration(address to, uint256 id) internal view override returns (bool status_) {
-        return _additionalConditionForOwnerTokenEnumeration(to, id);
+        status_ = _additionalConditionForOwnerTokenEnumeration(to, id);
    }