Merging and Splitting of positions will break for certain tokens

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x4d63d9ff89f7d82162d97b30dbcf04b6a83a30267e361d2a3b65c4509b9fe8dc Severity: high

Description: Description:

In the Router contract, the mergePositions() and redeemPositions() functions use transfer to transfer the collateralToken to the sender. Additionally, transferFrom() is used in the splitPosition() function. This will break for tokens that do not revert but return false on failure of transfer. The most well known ones of these being EURS, an euro stablecoin with over 120M EUR in circulation.

Attack Scenario:

Tokens not compliant with the ERC20 specification could return false from the transfer function call to indicate the transfer fails, while the calling contract would not notice the failure if the return value is not checked. Checking the return value is a requirement, as written in the EIP-20 specification: "Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!". As a result, a user's position could be merged, but his token would never be transferred to him. This would result in him losing funds.

Attachments

Use OpenZeppelin SafeERC20

greenlucid commented 2 months ago

Dupe https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/16

clesaege commented 2 months ago

rotcivegaf commented 2 months ago

The fact is that I sent it first, the transaction was the https://etherscan.io/tx/0xbbbbb07a00edddea303d7fe0d267c3023806831896192622fd2a194da6169ba4 Due to a bug in the hats app the issue was not created, when I spoke to Ofir he asked me to submit again

clesaege commented 2 months ago

Ok, I'm asking the hat team which one was sent first.

fonstack commented 2 months ago

I confirm @rotcivegaf information.

He submitted the issue on Sep-25-2024 03:01:11 PM UTC

https://etherscan.io/tx/0xbbbbb07a00edddea303d7fe0d267c3023806831896192622fd2a194da6169ba4

{
"isEncryptedByHats": false,
"decrypted": "**Project Name:** SeeR-PM\n\n**Project Id:** 0x899bc13919880db76edf4ccd72bdfa5dfa666fb7\n\n**Github username:** ---\n\n**Twitter username:** ---\n    \n    \n## [ISSUE #1]: Merging and Splitting of positions will break for certain tokens (high)\n\nDescription:\n\nIn the `Router` contract, the `mergePositions()` and `redeemPositions()` functions use `transfer` to transfer the `collateralToken` to the sender. Additionally, `transferFrom()` is used in the `splitPosition()` function. This will break for tokens that do not revert but return false on failure of transfer. The most well known ones of these being [EURS](https://etherscan.io/token/0xdb25f211ab05b1c97d595516f45794528a807ad8#code), an euro stablecoin with over 120M EUR in circulation.\n\nAttack Scenario:\n\nTokens not compliant with the ERC20 specification could return false from the transfer function call to indicate the transfer fails, while the calling contract would not notice the failure if the return value is not checked. Checking the return value is a requirement, as written in the EIP-20 specification: \"Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!\". As a result, a user's position could be merged, but his token would never be transferred to him. This would result in him losing funds.\n\nAttachments\n\nUse [OpenZeppelin SafeERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol)\n\n##",
"encrypted": "-----BEGIN PGP MESSAGE-----\n\nwcBMA845RwUsQiezAQgAqsU6fZearBRUZRnRbSwqDJChFn4aWOXUhkMN+12I\n47c5s3vTyRqHwe6PPELBbrfYRbP/EmPXSGPgYvnoTIWQXraYZ7fWsOlIhwgd\nWlbdl8spEOA+XnyBHl75Q21X2JJ0g8KPyrHt18/EuUX1As3iH3PnKyiCGz82\nJFGEkkySdY+1hnyHRNOrCRYoBgLTnNyDFpF3TDWCUvadYlSEFExwI1Rh2nxu\n5W9FF1U4YuxH5VbdQNik7IjUMIpCHpVomKtpGbqqksMjC6W437WNJhxq23Dl\nybqnL+y4yt0eGSSAEnlmrsZZJpT0v0MMy9t81tH1LrZC+RYArtk1cIxE8v7s\nmcHASwM7mMhlw/SWewEH+NAatrg3qkCbtjEMnCu/ksww3+pNCxyoBxuWrFvJ\nr/c3MPj/M0IUBPNyWhUiaAdnJ4W/tK0xertXkwMjATtG0ZdvIlVpItVrdDqr\n+kAKPEsfcrPPg+ta8JU8ah2ymt+rj4BB0UEWSKRiYGHjLjDiF6bY9MYYBaO/\nkqrSzIy1QC2GUtRlwn/svvfAXjIMk24/b25qZRaj+mH4LJIFp13J3i4dZ47/\nMd32CzuvaTx1/2sxVuvGnmVm6zwVEGEtdk+NBshHfty1bNCRZmrJBZxK7ts3\nGDaWRLejGlq1F00DaU1dIiVutDXalLECQ2oZB2uBgV1IphSoeEH37DzQOXUC\nsdKlAd6QXfQnM5wDtiSn6naeheoISgxC50oyvLdCNUwi4Ztn0pASHz7R7oEr\n5wN8wATYkOfpdVn3INQdDE1j+56bPxIDerxxS2bgpgC4ua/IfhfuAzhXMJKj\njVNxB2nBeeq9prfpfbwRMuJdsGDh6/YY5lZoOuLyNqrmgg1vumJLxoDsLIZD\n+QKk3Kd9HotOC41IO6z1KjOmLD8ULHYQ6ggLJ3SLBGX/\n=8YoV\n-----END PGP MESSAGE-----\n"
}

clesaege commented 2 months ago

Ok, I switched the reward to this one.

dinkras commented 1 month ago

Thanks for the input: @fonstack, Since such timing mismatch issues/UI bugs might occur in the future also, I think It would be very beneficial if you share: 1) How from the on-chain submission's _descriptionHash field can be verified that the encrypted/decrypted fields inside the JSON matches this hash? 2) How can we find other's auditors submissions on-chain, based on the GitHub issues data(for example Submission hash)?

I'm asking those questions because it looks that the github's issue creation time is not the source of truth for the first come first serve model.

clesaege commented 1 month ago

So let's categorize the ERC20 functions which should and shouldn't be checked.

To check:

transfer L80: if transfer fails, tokens get stuck in contract
transfer L95: if transfer fails, tokens get stuck in contract
transfer L133: if transfer fails, tokens get stuck in contract
transfer L157: if transfer fails, tokens get stuck in contract
transfer L210: if transfer fails, tokens get stuck in contract

Not to check:

transferFrom L62: If the transfer fails, splitting will fail anyways.
transferFrom L40: If the transfer fails, splitting will fail anyways.
transferFrom L117: If the transfer fails, merging will fail anyways.
transferFrom L182: If the transfer fails, redemption will fail anyways.

clesaege commented 1 month ago

Fixed in https://github.com/seer-pm/demo/pull/46

rotcivegaf commented 1 month ago

The wrapped1155 is created here and the implementation use openzeppelin's implementation so is a know token and don't have problem, so this checks there are not necessary:

transfer L80 wrapped1155.transfer(msg.sender, amount);
transfer L133 wrapped1155.transfer(msg.sender, amount);
transfer L210 parentWrapped1155.transfer(msg.sender, finalBalance - initialBalance);

About the collateralToken.transferFrom(msg.sender, address(this), amount);, I think it is necessary, apart from the fact that it is good practice, it could be an unknown token so it could return ‘false’ the transferFrom was not performed

hats-finance / SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7

Merging and Splitting of positions will break for certain tokens #103