Timeswap contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/Pool.sol#L679

Vulnerability details

Impact

The important states including long0Balance, long1Balance, long1FeeGrowth, long1ProtocolFees are wrongly calculated and it breaks the pool's invariant.

Proof of Concept

The protocol provides a rebalancing functionality and the main logic is implemented in the library Pool.sol. If param.isLong0ToLong1 is true and the transaction is TimeswapV2PoolRebalance.GivenLong1, the protocol calculates the long1AmountAdjustFees first and the actual long0Amount, longFees and the final long1Balance is decided accordingly. The problem is it is using the wrong parameter pool.long0Balance while it is supposed to use pool.long1Balance in the line L679.

This leads to wrong state calculation in the following logic. (especially L685 is setting the long1Balance to zero). Furthermore, the protocol is designed as a permission-less one and anyone can call TimeswapV2Pool.rebalance(). An attacker can abuse this to break the pool's invariant and take profit leveraging that.

packages\v2-pool\src\structs\Pool.sol
665:     function rebalance(Pool storage pool, TimeswapV2PoolRebalanceParam memory param, uint256 transactionFee, uint256 protocolFee) external returns (uint256 long0Amount, uint256 long1Amount) {
666:         if (pool.liquidity == 0) Error.requireLiquidity();
667:
668:         // No need to update short fee growth.
669:
670:         uint256 longFees;
671:         if (param.isLong0ToLong1) {
672:             if (param.transaction == TimeswapV2PoolRebalance.GivenLong0) {
673:                 (long1Amount, longFees) = ConstantSum.calculateGivenLongIn(param.strike, long0Amount = param.delta, transactionFee, true);
674:
675:                 if (long1Amount == 0) Error.zeroOutput();
676:
677:                 pool.long1Balance -= (long1Amount + longFees);
678:             } else if (param.transaction == TimeswapV2PoolRebalance.GivenLong1) {
    //************************************************************
679:                 uint256 long1AmountAdjustFees = FeeCalculation.removeFees(pool.long0Balance, transactionFee);//@audit-info long0Balance -> long1Balance
    //************************************************************
680:
681:                 if ((long1Amount = param.delta) == long1AmountAdjustFees) {
682:                     long0Amount = ConstantSum.calculateGivenLongOutAlreadyAdjustFees(param.strike, pool.long1Balance, true);
683:
684:                     longFees = pool.long1Balance.unsafeSub(long1Amount);
685:                     pool.long1Balance = 0;
686:                 } else {
687:                     (long0Amount, longFees) = ConstantSum.calculateGivenLongOut(param.strike, long1Amount, transactionFee, true);
688:
689:                     pool.long1Balance -= (long1Amount + longFees);
690:                 }
691:
692:                 if (long0Amount == 0) Error.zeroOutput();
693:             }
694:
695:             pool.long0Balance += long0Amount;
696:
697:             (pool.long1FeeGrowth, pool.long1ProtocolFees) = FeeCalculation.update(pool.liquidity, pool.long1FeeGrowth, pool.long1ProtocolFees, longFees, protocolFee);
698:         } else {
699:             if (param.transaction == TimeswapV2PoolRebalance.GivenLong0) {
700:                 uint256 long0AmountAdjustFees = FeeCalculation.removeFees(pool.long0Balance, transactionFee);//@audit-info
701:
702:                 if ((long0Amount = param.delta) == long0AmountAdjustFees) {
703:                     long1Amount = ConstantSum.calculateGivenLongOutAlreadyAdjustFees(param.strike, pool.long0Balance, false);
704:
705:                     longFees = pool.long0Balance.unsafeSub(long0Amount);
706:                     pool.long0Balance = 0;
707:                 } else {
708:                     (long1Amount, longFees) = ConstantSum.calculateGivenLongOut(param.strike, long0Amount, transactionFee, false);
709:
710:                     pool.long0Balance -= (long0Amount + longFees);
711:                 }
712:
713:                 if (long1Amount == 0) Error.zeroOutput();
714:             } else if (param.transaction == TimeswapV2PoolRebalance.GivenLong1) {
715:                 (long0Amount, longFees) = ConstantSum.calculateGivenLongIn(param.strike, long1Amount = param.delta, transactionFee, false);
716:
717:                 if (long0Amount == 0) Error.zeroOutput();
718:
719:                 pool.long0Balance -= (long0Amount + longFees);
720:             }
721:
722:             pool.long1Balance += long1Amount;
723:
724:             (pool.long0FeeGrowth, pool.long0ProtocolFees) = FeeCalculation.update(pool.liquidity, pool.long0FeeGrowth, pool.long0ProtocolFees, longFees, protocolFee);
725:         }
726:     }

Tools Used

Manual Review

Recommended Mitigation Steps

Fix the L679 as below.

uint256 long1AmountAdjustFees = FeeCalculation.removeFees(pool.long1Balance, transactionFee);

Lines of code

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L154-L163 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L36 https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L113-L117

Vulnerability details

Impact

In this protocol, all positions should have unique ids to track and update their status.

Currently, different positions might be minted to the same id and the main logic for the positions will be broken.

Proof of Concept

TimeswapV2LiquidityToken.mint() sets the id for new positions using totalSupply() + 1.

function mint(TimeswapV2LiquidityTokenMintParam calldata param) external returns (bytes memory data) {
    ParamLibrary.check(param);

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

    bytes32 key = timeswapV2LiquidityTokenPosition.toKey();
    uint256 id = _timeswapV2LiquidityTokenPositionIds[key];

    // if the position does not exist, create it
    if (id == 0) {
        id = totalSupply() + 1;
        _timeswapV2LiquidityTokenPositions[id] = timeswapV2LiquidityTokenPosition;
        _timeswapV2LiquidityTokenPositionIds[key] = id;
    }

And ERC1155Enumerable.totalSupply() returns the length of _allTokens.

    function totalSupply() public view override returns (uint256) {
        return _allTokens.length;
    }

But as we can see from ERC1155Enumerable._removeTokenFromAllTokensEnumeration(), the length of _allTokens can be decreased with the zero total supply token.

function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private {
    uint256 lastTokenIndex = _allTokens.length - 1;
    uint256 tokenIndex = _allTokensIndex[tokenId];
    uint256 lastTokenId = _allTokens[lastTokenIndex];

    _allTokens[tokenIndex] = lastTokenId;
    _allTokensIndex[lastTokenId] = tokenIndex;
    delete _allTokensIndex[tokenId];
    _allTokens.pop();
}

So the below scenario would be possible.

1. 100 different positions were minted and _allTokens contained 100 tokens.
2. Let's assume _timeswapV2LiquidityTokenPositionIds[key100] = 100 for the 100th position in TimeswapV2LiquidityToken.mint().
3. After that, the 50th token with 0 total supply was removed in ERC1155Enumerable._removeTokenFromAllTokensEnumeration() and the length of _allTokens was 99.
4. Now the 101th position was minted and _timeswapV2LiquidityTokenPositionIds[key101] = 100 again in TimeswapV2LiquidityToken.mint() because totalSupply() = 99.

So key100 and key101 for different positions have the same id and several functions using _timeswapV2LiquidityTokenPositionIds mapping like transferTokenPositionFrom() will be broken.

It's will be same in TimeswapV2Token.sol as well.

Tools Used

Manual Review

Recommended Mitigation Steps

ERC1155Enumerable.totalSupply() mustn't be decreased in any case.

It should return the maximum id in _allTokens array or we can introduce a totalsupply variable that increases all the time and use it.

Lines of code

https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L94-L95

Vulnerability details

Impact

ERC1155Enumerable._removeTokenEnumeration() checks the removal condition wrongly.

As a result, the tokens with 0 total supply won't be removed from _allTokens array at all.

Proof of Concept

_removeTokenEnumeration() checks the removal condition like below when to == address(0).

function _removeTokenEnumeration(address from, address to, uint256 id, uint256 amount) internal {
    if (to == address(0)) {
        if (_idTotalSupply[id] == 0 && _additionalConditionRemoveTokenFromAllTokensEnumeration(id)) _removeTokenFromAllTokensEnumeration(id); //@audit _idTotalSupply[id] == amount
        _idTotalSupply[id] -= amount;
    }

Currently it checks if _idTotalSupply[id] == 0 before substrating amount and it will be false always as _idTotalSupply[id] >= amount and amount > 0.

So the tokens with 0 total supply won't be removed all the time.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend modifying like below.

function _removeTokenEnumeration(address from, address to, uint256 id, uint256 amount) internal {
    if (to == address(0)) {
        if (_idTotalSupply[id] == amount && _additionalConditionRemoveTokenFromAllTokensEnumeration(id)) _removeTokenFromAllTokensEnumeration(id);
        _idTotalSupply[id] -= amount;
    }

[L-01] Option.transferPosition() works with any amount when msg.sender == to

• https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Option.sol#L64-L66

    function transferPosition(Option storage option, address to, TimeswapV2OptionPosition position, uint256 amount) internal {
        if (position == TimeswapV2OptionPosition.Long0) {
            option.long0[msg.sender] -= amount;
            option.long0[to] += amount;
        } else if (position == TimeswapV2OptionPosition.Long1) {
            option.long1[to] += amount; //@audit should minus first
            option.long1[msg.sender] -= amount;
        } else if (position == TimeswapV2OptionPosition.Short) {
            option.short[msg.sender] -= amount;
            option.short[to] += amount;
        }
    }

Currently, it adds amount first for the TimeswapV2OptionPosition.Long1 position so the function will work properly with any amount if msg.sender == to.

So TimeswapV2Option.transferPosition() will emit a wrong event.

[L-02] Lack of checks-effects-interactions

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

It's recommended to execute external calls after state changes, to prevent reetrancy bugs.

[L-03] Wrong comments

• https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/structs/Process.sol#L32

/// @param isAddToken1 IsAddToken1 if true. IsSubToken0 if false.

IsSubToken0 should be IsSubToken1.

[N-01] Incorrect event name

• https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/base/OwnableTwoSteps.sol#L31

emit SetOwner(pendingOwner);

It should be SetPendingOwner, otherwise, it might make users confusing.