Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow swapping the same token #89

Open
code423n4 opened this issue Nov 7, 2021 · 2 comments
Open

Don't allow swapping the same token #89

code423n4 opened this issue Nov 7, 2021 · 2 comments
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

Ruhum

Vulnerability details

Impact

A user might mistakenly swap the same two tokens resulting in them losing funds due to the fees, etc.

I think we can assume that no user would really want to swap the same token. So that might as well be prohibited by checking the passed token indexes in the respective functions.

Uniswap for example doesn't allow creating a pool with the same two tokens, see here

Proof of Concept

Here are the relevant functions IMO:

https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/Swap.sol#L356

https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/Swap.sol#L461

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add a require statement:
`require(tokenIndexFrom != tokenIndexTo, "swapping the same token is not allowed");

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 7, 2021
code423n4 added a commit that referenced this issue Nov 7, 2021
@chickenpie347 chickenpie347 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Nov 16, 2021
@chickenpie347
Copy link
Collaborator

While this is technically correct, the UI would restrict same-token swaps - and the users who do use SC interaction to swap, likely know better. Adding that statement for a fringe case unnecessarily increases gas costs for all users.

@0xean 0xean added invalid This doesn't seem right 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments and removed invalid This doesn't seem right labels Jan 8, 2022
@0xean 0xean reopened this Jan 8, 2022
@0xean
Copy link
Collaborator

0xean commented Jan 8, 2022

Reopening as a low risk, as the check could be added to mitigate some unintentional user behavior, but the cost is high for all users due to additional gas on every swap.

@0xean 0xean removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants