Timeswap contest - SaeedAlipoor01988's results

Lines of code

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L133

Vulnerability details

Impact

whitepaper : 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.

but TimeswapV2Option does not appear to support fee-on-transfer tokens whose charge amount of tokens on transactions.

Proof of Concept

in TimeswapV2Option and mint function, https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L133

we ask the msg.sender to transfer token0 and/or token1 to this contract. and amounts for token 0 and token 1 are provided by contract. after token transfer in below https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L145 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L148

we check that balance targets are achieved.

now, if token 0 or token 1 are fee-on-transfer tokens, for transfer token0AndLong0Amount or token1AndLong1Amount, msg.sender should pay the fee and the final received amount to TimeswapV2Option is not target balances. and the transaction will get reverted.

same for swap : https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L198

Tools Used

manually

Recommended Mitigation Steps

we should first get the amount from the user and based on the received amount, call mint for the user.

Missing validation in constructors TimeswapV2Token misses zero-address validation on the address of the chosenOptionFactory set in the constructor. There is no other function to change the address of chosenOptionFactory in the TimeswapV2Token contract. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2Token.sol#L42

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L36 The same is found here: https://github.com/code-423n4/2022-01-trader-joe-findings/issues/263 https://github.com/code-423n4/2022-01-sandclock-findings/issues/107 ////////////////////////////////////////////// ***** ////////////////////////////////////////////// Missing same address check https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L75 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L152

////////////////////////////////////////////// ***** //////////////////////////////////////////////

in TimeswapV2PoolFactory contract and function create, you are returning one error for two separate subjects. in the first line, if the input address is zero address, the function will return an Error.zeroAddress(); https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2PoolFactory.sol#L60

but in the next line, we get pair from pairs mapping, and if the pair is zero address again function will return Error.zeroAddress();

but in this line, I think we should return this error, Error.pairalreadyexist();

////////////////////////////////////////////// ***** //////////////////////////////////////////////

in transferLiquidity function, https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L152

user A wants to transfer liquidityAmount from his balance to user B's balance. but we don't check at top of the function body that, user A has enough liquidity and directly we call

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

but we can manage this scenario more easily,

liquidityAmountToTransfer = liquidityAmount > pools[strike][maturity].liquidityPositions[msg.sender].liquidity ? pools[strike][maturity].liquidityPositions[msg.sender].liquidity : liquidityAmount ;

and use liquidityAmountToTransfer in burn and mint, instead of using the user's input liquidityAmount .

same for : https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L165

////////////////////////////////////////////// ***** //////////////////////////////////////////////

Omissions in Events with old value Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

The events should include the new value and old value where possible: https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/base/OwnableTwoSteps.sol#L41

////////////////////////////////////////////// ***** //////////////////////////////////////////////

in transferLiquidity function, https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L153

at the first line, we check that the matching pool has liquidity of more than zero. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L98

then w will call transferLiquidity function from the pool library https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/Pool.sol#L158

again at the first line of this function, we have and if block to check if the pool has liquidity.

this is double-checking!

////////////////////////////////////////////// ***** //////////////////////////////////////////////

Multiple bytes32 mappings can be combined into a single mapping of a bytes32 to a struct, where appropriate Saves a storage slot for mapping. Depending on the circumstances and sizes of types, can avoid a Gusset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2Token.sol#L36 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2Token.sol#L39

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L41 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L45

////////////////////////////////////////////// ***** //////////////////////////////////////////////

require() or revert() or if () statements that check input arguments should be at the top of the function Checks that involve constants should come before checks that involve state variables, function calls, and calculations.

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L75

we need to first check that from address != to address at the top of the function.

////////////////////////////////////////////// ***** //////////////////////////////////////////////

In the TimeswapV2LiquidityToken contract, and in the function transferFeesFrom, we are getting input with the type of TimeswapV2LiquidityTokenPosition, position. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L75

We will compute the id from this input, position, with the help of _timeswapV2LiquidityTokenPositionIds mapping. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L81

In the next lines, we will pass the computed id to function _updateFeesPositions, and in this function, we are using the inputted id to compute the matched position from timeswapV2LiquidityTokenPosition mapping. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L88 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L238

Why don’t we simply pass the position input from transferFeesFrom to the updateFeesPositions?

In the collect function, we get the param as input. From the param, we compute the byte32 position. And from the byte32 position we will get id. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L182 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L185 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L195

Then we pass the id to the updateFeesPositions and in this function again we will compute the byte32 position. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L197

In a better state, we can Change updateFeesPositions to the

function _updateFeesPositions(address from, address to, TimeswapV2LiquidityTokenPosition calldata position)

and pass position directly from the transferFeesFrom to updateFeesPositions! to prevent reading from storage again! When we have value.

and compute and pass timeswapV2LiquidityTokenPosition to the updateFeesPositions, from the collect function.

    TimeswapV2LiquidityTokenPosition memory timeswapV2LiquidityTokenPosition = TimeswapV2LiquidityTokenPosition({
        token0: param.token0,
        token1: param.token1,
        strike: param.strike,
        maturity: param.maturity
    });

And in _beforeTokenTransfer, pass _timeswapV2LiquidityTokenPositions[id] in for loop.

    for (uint256 i; i < ids.length; ) {
        if (amounts[i] != 0) _updateFeesPositions(from, to, _timeswapV2LiquidityTokenPositions[ids[i]

]);

        unchecked {
            ++i;
        }
    }

////////////////////////////////////////////// ***** //////////////////////////////////////////////

use x = x + y, instead of x += y;

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L51 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L66 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L69 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L75

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Option.sol#L60 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Option.sol#L85 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Option.sol#L127

////////////////////////////////////////////// ***** //////////////////////////////////////////////

in some functions, we call Transfer position. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Option.sol#L60

but before calling this function, we need to check that caller has enough balance to transfer. . By doing these checks first, the function is able to revert before wasting a Gcoldsload.

for example in : https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L97

by checking the balance of msg.sender in the function https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2Option.sol#L97

before making the call to transferPosition function, we can add unchecked in to transferPosition function.