hats-finance / SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7

1 stars 0 forks source link

Incorrect Amount in _redeemPositions #140

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xdac4932b4ba13934ec992da8bfe3e8db012c23c68f4bb021711f9af8da0fcff6 Severity: medium

Description:

Details

The Router contract enables users to interact with Conditional Token markets. The _redeemPositions function allows users to redeem their winning outcome tokens for the underlying collateral or parent outcome tokens after a market resolves.

The _redeemPositions function incorrectly assumes that the user wants to redeem their entire balance of a specific outcome token. It uses wrapped1155.balanceOf(msg.sender) to determine the amount to redeem. However, the user might only want to redeem a portion of their holdings. This could lead to unexpected behavior and incorrect redemption amounts.

Code Snippet


// ... inside _redeemPositions loop ...

// Incorrect amount calculation:
uint256 amount = wrapped1155.balanceOf(msg.sender); 

wrapped1155.transferFrom(msg.sender, address(this), amount);
wrapped1155Factory.unwrap(address(conditionalTokens), tokenId, amount, address(this),    data);

Impact

This bug forces users to redeem their entire balance of an outcome token, even if they only want to redeem a part of it. This limits user flexibility and could lead to unintended consequences.

Scenario

Fix

Instead of using wrapped1155.balanceOf(msg.sender), the function should accept an amount parameter to allow users to specify the exact amount of each outcome token they want to redeem.


// ... inside _redeemPositions ...

// Add amount parameter to the function signature:
function _redeemPositions(
    IERC20 collateralToken, 
    Market market, 
    uint256[] calldata outcomeIndexes,
    uint256[] calldata amounts // Add amounts array
) internal { 
    // ...

    // Use the provided amount from the amounts array:
    uint256 amount = amounts[j]; 

    wrapped1155.transferFrom(msg.sender, address(this), amount);
    wrapped1155Factory.unwrap(address(conditionalTokens), tokenId, amount, address(this),    data);
    // ...
}

Test


it("should redeem only the specified amount of tokens", async function () {
  const ANSWER = 1;
  const REDEEMED_POSITION = 1;
  const [owner] = await ethers.getSigners();
  // split first
  const { outcomeSlotCount, conditionId, questionsIds, market } = await createMarketAndSplitPosition();

  // answer the question and resolve the market
  await time.increase(OPENING_TS);
  await realitio.submitAnswer(questionsIds[0], ethers.toBeHex(BigInt(ANSWER), 32), 0, {
    value: ethers.parseEther(MIN_BOND),
  });
  await time.increase(QUESTION_TIMEOUT);
  await realityProxy.resolve(market);

  // allow router to transfer position tokens to the contract
  for (let i = 0; i < outcomeSlotCount; i++) {
    const [wrapped1155] = await market.wrappedOutcome(i);
    const token = await ethers.getContractAt("Wrapped1155", wrapped1155);
    await token.approve(router, ethers.parseEther(SPLIT_AMOUNT));
  }

  // Redeem only half the amount
  const redeemAmount = ethers.parseEther(String(Number(SPLIT_AMOUNT) / 2));
  await router.redeemPositions(collateralToken, market, [REDEEMED_POSITION], [redeemAmount]);

  const [wrapped1155] = await market.wrappedOutcome(REDEEMED_POSITION);
  const token = await ethers.getContractAt("Wrapped1155", wrapped1155);

  // Check that only half the amount was redeemed
  expect(await token.balanceOf(owner)).to.equal(redeemAmount); 
});

This test sets up a scenario where the user wants to redeem only half of their winning outcome tokens. It then checks the balance after calling redeemPositions with the specified amount to ensure that only the intended amount was redeemed. This verifies that the fix allows for partial redemptions.

clesaege commented 1 month ago

Why would a user want to only redeem part of their holdings? This function is a router function to redeem all your tokens when the market is resolved. This looks more like a feature request than a vuln. Moreover, we already added a amount parameter as a result of https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/82#issuecomment-2391361721