Each leg in a tokenid has a risk partner which is usually its own index but in some cases, it could be another leg (Partner in defined risk position).
In the function validate() if the risk partner of a specific leg is not its own index then some additional checks are done to ensure that they are compatible like
Ensuring that risk partners are mutual
Ensures that risk partners have the same asset.
Ensures that risk partners have the same ratio.
Plus other checks that depend on the legs isLong value compared to that of its risk partner.
function validate(TokenId self) internal pure {
if (self.optionRatio(0) == 0) revert Errors.InvalidTokenIdParameter(1);
// More Code...
// In the following, we check whether the risk partner of this leg is itself
// or another leg in this position.
// Handles case where riskPartner(i) != i ==> leg i has a risk partner that is another leg
uint256 riskPartnerIndex = self.riskPartner(i);
if (riskPartnerIndex != i) {
// Ensures that risk partners are mutual
if (self.riskPartner(riskPartnerIndex) != i)
revert Errors.InvalidTokenIdParameter(3);
// Ensures that risk partners have 1) the same asset, and 2) the same ratio
if (
(self.asset(riskPartnerIndex) != self.asset(i)) ||
(self.optionRatio(riskPartnerIndex) != self.optionRatio(i))
) revert Errors.InvalidTokenIdParameter(3);
// long/short status of associated legs
uint256 _isLong = self.isLong(i);
uint256 isLongP = self.isLong(riskPartnerIndex);
// token type status of associated legs (call/put)
uint256 _tokenType = self.tokenType(i);
uint256 tokenTypeP = self.tokenType(riskPartnerIndex);
// if the position is the same i.e both long calls, short put's etc.
// then this is a regular position, not a defined risk position
if ((_isLong == isLongP) && (_tokenType == tokenTypeP))
revert Errors.InvalidTokenIdParameter(4);
// if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position
// A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally
// unlike short stranlges, long strangles also cannot be partnered, because there is no reduction in risk (both legs can earn premia simultaneously)
if (((_isLong != isLongP) || _isLong == 1) && (_tokenType != tokenTypeP))
revert Errors.InvalidTokenIdParameter(5);
}
} // end for loop over legs
}
}
In `burnTokenizedPosition()` the internal function `_validateAndForwardToAMM()` is called, this function calls [Tokenid.flipToBurnToken()](https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/types/TokenId.sol#L366-L398) which simply `flips` the isLong bits of all active legs of the tokenid. then `validate()` is called which validates a position tokenId and its legs.
```solidity
/// @param tokenId the option position
/// @param positionSize the size of the position to create
/// @param tickLimitLow lower limits on potential slippage
/// @param tickLimitHigh upper limits on potential slippage
/// @param isBurn is equal to false for mints and true for burns
/// @return collectedByLeg An array of LeftRight encoded words containing the amount of token0 and token1 collected as fees for each leg
/// @return totalMoved the total amount of funds swapped in Uniswap as part of building potential ITM positions
function _validateAndForwardToAMM(
TokenId tokenId,
uint128 positionSize,
int24 tickLimitLow,
int24 tickLimitHigh,
bool isBurn
) internal returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalMoved) {
// Reverts if positionSize is 0 and user did not own the position before minting/burning
if (positionSize == 0) revert Errors.OptionsBalanceZero();
/// @dev the flipToBurnToken() function flips the isLong bits
if (isBurn) {
tokenId = tokenId.flipToBurnToken();
}
// Validate tokenId
tokenId.validate();
// Extract univ3pool from the poolId map to Uniswap Pool
IUniswapV3Pool univ3pool = s_poolContext[tokenId.poolId()].pool;
// Revert if the pool not been previously initialized
if (univ3pool == IUniswapV3Pool(address(0))) revert Errors.UniswapPoolNotInitialized();
// More Code...
}
The issue here is that if a leg in the tokenid has its risk partner as another leg (that is, it is not its own risk partner), then flipping the isLong bits may cause one of the checks
in validate() to fail and revert as the isLong bits of its risk partner are not changed as well.
Remember that flipping changes the value of the bit from what it was to an opposite value (from 0 to 1 or from 1 to 0).
For example;
Let's say a leg with a different risk partner has isLong() values that are the same but their tokenType() is different, this would easily pass these checks below from validate()
but after a flip is done to its isLong bits using flipToBurnToken() it will fail and revert in the second check below.
// if the position is the same i.e both long calls, short put's etc.
// then this is a regular position, not a defined risk position
if ((_isLong == isLongP) && (_tokenType == tokenTypeP))
revert Errors.InvalidTokenIdParameter(4);
// if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position
// A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally
// unlike short stranlges, long strangles also cannot be partnered, because there is no reduction in risk (both legs can earn premia simultaneously)
if (((_isLong != isLongP) || _isLong == 1) && (_tokenType != tokenTypeP))
revert Errors.InvalidTokenIdParameter(5);
Impact
This will result in a continuous revert of the function leading to an inability to Burn a Tokenized Position.
Tools Used
Manual Review
Recommended Mitigation Steps
This whole issue results from the simple fact the risk partners, if different are not flipped as well, I recommend validating the tokenid before flipping the isLong bits, to ensure any changes caused by flipping will not affect the execution of the function.
/// @param tokenId the option position
/// @param positionSize the size of the position to create
/// @param tickLimitLow lower limits on potential slippage
/// @param tickLimitHigh upper limits on potential slippage
/// @param isBurn is equal to false for mints and true for burns
/// @return collectedByLeg An array of LeftRight encoded words containing the amount of token0 and token1 collected as fees for each leg
/// @return totalMoved the total amount of funds swapped in Uniswap as part of building potential ITM positions
function _validateAndForwardToAMM(
TokenId tokenId,
uint128 positionSize,
int24 tickLimitLow,
int24 tickLimitHigh,
bool isBurn
) internal returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalMoved) {
// Reverts if positionSize is 0 and user did not own the position before minting/burning
if (positionSize == 0) revert Errors.OptionsBalanceZero();
++ // Validate tokenId
++ tokenId.validate();
/// @dev the flipToBurnToken() function flips the isLong bits
if (isBurn) {
tokenId = tokenId.flipToBurnToken();
}
// Extract univ3pool from the poolId map to Uniswap Pool
IUniswapV3Pool univ3pool = s_poolContext[tokenId.poolId()].pool;
// Revert if the pool not been previously initialized
if (univ3pool == IUniswapV3Pool(address(0))) revert Errors.UniswapPoolNotInitialized();
// More Code...
}
Lines of code
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/types/TokenId.sol#L535-L571 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L673-L702
Vulnerability details
Each leg in a
tokenid
has a risk partner which is usually its own index but in some cases, it could be another leg (Partner in defined risk position). In the functionvalidate()
if the risk partner of a specific leg isnot
its own index then some additional checks are done to ensure that they are compatible likeisLong
value compared to that of its risk partner.// More Code... // In the following, we check whether the risk partner of this leg is itself // or another leg in this position. // Handles case where riskPartner(i) != i ==> leg i has a risk partner that is another leg uint256 riskPartnerIndex = self.riskPartner(i); if (riskPartnerIndex != i) { // Ensures that risk partners are mutual if (self.riskPartner(riskPartnerIndex) != i) revert Errors.InvalidTokenIdParameter(3);
The issue here is that if a leg in the tokenid has its
risk partner
as another leg (that is, it is not its own risk partner), then flipping theisLong
bits may cause one of the checks invalidate()
to fail and revert as theisLong
bits of its risk partner arenot
changed as well.Remember that flipping changes the value of the bit from what it was to an opposite value (from 0 to 1 or from 1 to 0).
For example; Let's say a leg with a different risk partner has
isLong()
values that are the same but theirtokenType()
is different, this would easily pass these checks below fromvalidate()
but after a flip is done to itsisLong
bits using flipToBurnToken() it will fail and revert in the second check below.Impact
This will result in a continuous revert of the function leading to an inability to Burn a Tokenized Position.
Tools Used
Manual Review
Recommended Mitigation Steps
This whole issue results from the simple fact the risk partners, if different are not flipped as well, I recommend validating the
tokenid
before flipping theisLong
bits, to ensure any changes caused by flipping will not affect the execution of the function.Assessed type
Other