Timeswap contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The collect() function will always transfer ZERO fees. At the same time, non-zero _fessPosition will be burned.

_feesPositions[id][msg.sender].burn(long0Fees, long1Fees, shortFees);

As a result, the contracts will be left in an inconsistent state. The user will burn _feesPositions without receiving the the fees!

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. The collect() function will always transfer ZERO fees in the following line:

 // transfer the fees amount to the recipient
        ITimeswapV2Pool(poolPair).transferFees(param.strike, param.maturity, param.to, long0Fees, long1Fees, shortFees);

This is because, at this moment, the values of long0Fees, long1Fees, shortFees have not been calculated yet, actually, they will be equal to zero. Therefore, no fees will be transferred. The values of long0Fees, long1Fees, shortFees are calculated afterwards by the following line:

(long0Fees, long1Fees, shortFees) = _feesPositions[id][msg.sender].getFees(param.long0FeesDesired, param.long1FeesDesired, param.shortFeesDesired);

Therefore, ITimeswapV2Pool(poolPair).transferFees must be called after this line to be correct.

Tools Used

Remix

Recommended Mitigation Steps

We moved the line ITimeswapV2Pool(poolPair).transferFees after long0Fees, long1Fees, shortFees have been calculated first.

function collect(TimeswapV2LiquidityTokenCollectParam calldata param) external returns (uint256 long0Fees, uint256 long1Fees, uint256 shortFees, bytes memory data) {
        ParamLibrary.check(param);

        bytes32 key = TimeswapV2LiquidityTokenPosition({token0: param.token0, token1: param.token1, strike: param.strike, maturity: param.maturity}).toKey();

        // start the reentrancy guard
        raiseGuard(key);

        (, address poolPair) = PoolFactoryLibrary.getWithCheck(optionFactory, poolFactory, param.token0, param.token1);


        uint256 id = _timeswapV2LiquidityTokenPositionIds[key];

        _updateFeesPositions(msg.sender, address(0), id);

        (long0Fees, long1Fees, shortFees) = _feesPositions[id][msg.sender].getFees(param.long0FeesDesired, param.long1FeesDesired, param.shortFeesDesired);

        if (param.data.length != 0)
            data = ITimeswapV2LiquidityTokenCollectCallback(msg.sender).timeswapV2LiquidityTokenCollectCallback(
                TimeswapV2LiquidityTokenCollectCallbackParam({
                    token0: param.token0,
                    token1: param.token1,
                    strike: param.strike,
                    maturity: param.maturity,
                    long0Fees: long0Fees,
                    long1Fees: long1Fees,
                    shortFees: shortFees,
                    data: param.data
                })
            );

                // transfer the fees amount to the recipient
        ITimeswapV2Pool(poolPair).transferFees(param.strike, param.maturity, param.to, long0Fees, long1Fees, shortFees);


        // burn the desired fees from the fees position
        _feesPositions[id][msg.sender].burn(long0Fees, long1Fees, shortFees);

        if (long0Fees != 0 || long1Fees != 0 || shortFees != 0) _removeTokenEnumeration(msg.sender, address(0), id, 0);

        // stop the reentrancy guard
        lowerGuard(key);
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. _removeTokenEnumeration() will MOSTLY fail to remove any token from the list. The reason is that the function check _idTotalSupply[id] == 0 first before doing the deduction on _idTotalSupply[id].

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. _removeTokenEnumeration() will mostly fail to remove any tokens from the list. _removeTokenEnumeration() attempts to remove the token from _allTokens when _idTotalSupply[id] == 0, However, it will fail for sure because:

1. _removeTokenEnumeration() checks _idTotalSupply[id] == 0 first, and then does _idTotalSupply[id] -= amount, it should be the other way around.

2. if _idTotalSupply[id] == 0, then the next statement (see follows) will revert due to underflow:

            _idTotalSupply[id] -= amount;

3. if _idTotalSupply[id] != 0, then even after the next statement, it becomes to zero, there is no logic to remove the token at this point. Note the next round of calling _removeTokenEnumeration() won't help, as it will revert due to underflow as pointed above in 2) (unless amount == 0).

In summary, _removeTokenEnumeration() will MOSTLY fail to remove any tokens from the list.

Tools Used

Remix

Recommended Mitigation Steps

The fix is to do the subtraction first before the zero amount check.

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

        if (from != address(0) && from != to) {
            if (balanceOf(from, id) == 0 && _additionalConditionRemoveTokenFromOwnerEnumeration(from, id)) _removeTokenFromOwnerEnumeration(from, id);
        }
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The data structure _ownedTokensIndex is SHARED by different owners, as a result, _removeTokenFromAllTokensEnumeration() might remove the wrong tokenId.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

_ownedTokensIndex is used to map from token ID to index of the owner tokens list, unfortunately, all owners share the same data structure at the same time (non-fungible tokens). So, the mapping for one owner might be overwritten by another owner when _addTokenToOwnerEnumeration is called: https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L116-L121. As a result, _removeTokenFromOwnerEnumeration() might remove the wrong tokenID.

Removing the wrong tokenID can happen like the following:

1. Suppose Alice owns three tokens A, B, C with indices 1 -> A, 2->B, 3->C
2. Suppose Bob owns token D, 1->D, and will add A to his list via _addTokenToOwnerEnumeration(). As a result, we have 1->D, and 2-A, since _ownedTokensIndex is shared, we have A->2 in _ownedTokensIndex.
3. Next, _removeTokenFromOwnerEnumeration() is called to remove A from Alice. However, tokenIndex will be 2, which points to B, as a result, instead of deleting A, B is deleted from _ownedTokens. Wrong token delete!

function _removeTokenFromOwnerEnumeration(address from, uint256 tokenId) private {
        uint256 lastTokenIndex = _currentIndex[from] - 1;
        uint256 tokenIndex = _ownedTokensIndex[tokenId];

        if (tokenIndex != lastTokenIndex) {
            uint256 lastTokenId = _ownedTokens[from][lastTokenIndex];

            _ownedTokens[from][tokenIndex] = lastTokenId;
            _ownedTokensIndex[lastTokenId] = tokenIndex;
        }

        delete _ownedTokensIndex[tokenId];
        delete _ownedTokens[from][lastTokenIndex];
    }

Tools Used

Remix

Recommended Mitigation Steps

Redefine ownedTokensIndex so that is is not shared:

mapping(address => mapping(uint256 => uint256)) private _ownedTokensIndex;

QA1. transferFeesFrom() cannot be used by a user to send fees from himself to another user

https://github.com/code-423n4/2023-01-timeswap/blame/main/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L79

We can add the condition from == msg.sender as well to mitigate this issue:

if (from != msg.sender && !isApprovedForAll(from, msg.sender)) revert NotApprovedToTransferFees();

QA2. A user might mistakenly provide param.to with the address of the contract and lose fund

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

Mitigation: Adding a check param.to != address(this) to ensure not to lose fund. Adding the following check param.long0To != address(this) && param.long1To != address(this) && param.shortTo != address(this) revert ToThisContract(); to avoid losing fund.

QA3. transferFeesFrom() does not update _feesPositions properly

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

We need to bring _feesPositions up to date by calling _updateFeesPositions() first before burn() and mint(), therefore, we need to move _updateFeesPositions() before burn() and mint(). Otherwise, the fees will be calculated wrongly.

 _updateFeesPositions(from, to, id);          // @audit: updating fees must occur before the changes of liquidities

_feesPositions[id][to].mint(long0Fees, long1Fees, shortFees);

_feesPositions[id][from].burn(long0Fees, long1Fees, shortFees);

QA4. _removeTokenFromOwnerEnumeration REPLACES the wrong lastTokenId.

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

The function removes tokenId from the list of from by replacing it with lastTokenId. However the following statement of retrieving the lastTokenId is wrong:

 uint256 lastTokenIndex = _currentIndex[from] - 1;

This is because the last index used is _currentIndex[from], see the implementation: https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L116

The fix is as follows:

function _removeTokenFromOwnerEnumeration(address from, uint256 tokenId) private {
        uint256 lastTokenIndex = _currentIndex[from];   // @audit: fix here
        uint256 tokenIndex = _ownedTokensIndex[tokenId];

        if (tokenIndex != lastTokenIndex) {
            uint256 lastTokenId = _ownedTokens[from][lastTokenIndex];

            _ownedTokens[from][tokenIndex] = lastTokenId;
            _ownedTokensIndex[lastTokenId] = tokenIndex;
        }

        delete _ownedTokensIndex[tokenId];
        delete _ownedTokens[from][lastTokenIndex];
}

QA5: create() fails to push the new optionPair into array getByIndex, as a result, function numberOfPairs() will return the wrong value.

create() fails to push the new optionPair into array getByIndex, as a result, function numberOfPairs() will return the wrong value.

The revision is as follows.

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);

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

        optionPairs[token0][token1] = optionPair;

        getByIndex.push(optionPair);   // push the new ``optionPair`` into array ``getByIndex``

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

G1. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-option/src/TimeswapV2OptionFactory.sol#L49 Replacing it by a zero address check for optionPair will do it. That is what the function did anyway.

if (optionPair != address(0)) revert OptionPairAlreadyExisted(token0, token1, optionPair);

G2. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2PoolDeployer.sol#L26-L28 It will be more gas-efficient to pass the Parameter struct as a calldata to the newly created TimeswapV2Pool's constructor. Passing by state variable is two expensive.

G3. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L55 listOfPools is not used and can be deleted along with its associated functions.

G4. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L51-L63 We can save gas here by checking whether the last fee growth and the global fee growth are equal. If yes, 1) there is no need to revise the last fee growth, which is a state variable, and 2) there is no need to calculate and add the fee either.

function update(LiquidityPosition storage liquidityPosition, uint256 long0FeeGrowth, uint256 long1FeeGrowth, uint256 shortFeeGrowth) internal {
        uint160 liquidity = liquidityPosition.liquidity;

        if (liquidity !=0){
            uint256 lastGrowth = liquidityPosition.long0FeeGrowth;
            if(lastLong0FeeGrowth != long0FeeGrowth){ 
                      liquidityPosition.long0Fees += FeeCalculation.getFees(liquidity, lastGrowth, long0FeeGrowth);
                      liquidityPosition.long0FeeGrowth = long0FeeGrowth;
            }

            lastGrowth = liquidityPosition.long1FeeGrowth;
            if(lastGrowth != long1FeeGrowth){ 
                     liquidityPosition.long1Fees += FeeCalculation.getFees(liquidity, lastGrowth, long1FeeGrowth);
                     liquidityPosition.long1FeeGrowth = long1FeeGrowth;
           }
  
           lastGrowth = liquidityPosition.shortFeeGrowth;
           if(lastGrowth != shortFeeGrowth){
                    liquidityPosition.shortFees += FeeCalculation.getFees(liquidity, lastGrowth, shortFeeGrowth);
                    liquidityPosition.shortFeeGrowth = shortFeeGrowth;
           }
        }
    }

G5. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/structs/LiquidityPosition.sol#L33-L44 We can save gas here by checking whether the last fee growth and the global fee growth are equal

function feesEarnedOf(
        LiquidityPosition memory liquidityPosition,
        uint256 long0FeeGrowth,
        uint256 long1FeeGrowth,
        uint256 shortFeeGrowth
    ) internal pure returns (uint256 long0Fee, uint256 long1Fee, uint256 shortFee) {
        uint160 liquidity = liquidityPosition.liquidity;

        uint256 lastGrowth = liquidityPosition.long0FeeGrowth;
       if(lastGrowth != long0FeeGrowth){
           long0Fee = liquidityPosition.long0Fees.unsafeAdd(FeeCalculation.getFees(liquidity, lastGrowth, long0FeeGrowth));
       }

       lastGrowth = liquidityPosition.long1FeeGrowth;
       if(lastGrowth != long1FeeGrowth){ 
           long1Fee = liquidityPosition.long1Fees.unsafeAdd(FeeCalculation.getFees(liquidity, lastGrowth , long1FeeGrowth));
       }

       lastGrowth = liquidityPosition.shortFeeGrowth;
       if(lastGrowth != shortFeeGrowth){
            shortFee = liquidityPosition.shortFees.unsafeAdd(FeeCalculation.getFees(liquidity, lastGrowth , shortFeeGrowth));
       }
    }

G6. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-token/src/base/ERC1155Enumerable.sol#L154-L163 We can check whether tokenIndex == lastTokenIndex to save gas:

 function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private {
        uint256 lastTokenIndex = _allTokens.length - 1;
        uint256 tokenIndex = _allTokensIndex[tokenId];
        uint256 lastTokenId = _allTokens[lastTokenIndex];
  
        if(tokenIndex != lastTokenIndex){   // audit: save gas here
             _allTokens[tokenIndex] = lastTokenId;
             _allTokensIndex[lastTokenId] = tokenIndex;
        }
        delete _allTokensIndex[tokenId];
        _allTokens.pop();
    }

G7. https://github.com/code-423n4/2023-01-timeswap/blob/ef4c84fb8535aad8abd6b67cc45d994337ec4514/packages/v2-pool/src/TimeswapV2Pool.sol#L146 There are many instances of function call blockTimestamp(0), they can all be replaced by uint96(block.timestmap) to save the gas of function call and addition with zero.