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

Approve to 0 first to be compatible with USDT-like tokens #473

Closed
code423n4 opened this issue Aug 30, 2023 · 8 comments
Closed

Approve to 0 first to be compatible with USDT-like tokens #473

code423n4 opened this issue Aug 30, 2023 · 8 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1782 sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L200
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L204
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L328
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L169
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L173
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L302

Vulnerability details

Impact

If router spends less tokens than desired, there will some dust tokens left, and there will left allowance on ERC20 with this dust amount. Some tokens like USDT revert when set non-zero allowance from non-zero allowance.
In case when this situation occurs, and router doesn't spend all allowance, further interaction will be broken because of revert on approve()

Proof of Concept

I mentioned all the occurrences of vulnerable setting approval in "Links to affected code".
The context is following: UniV3LiquidityAmo.sol and UniV2LiquidityAmo.sol interact with Uniswap router, on every operation they set new allowance, but as described this can revert. As a result, contracts cannot perform further operations because of reverting approve

Tools Used

Manual Review

Recommended Mitigation Steps

Firstly make force approve to zero

Assessed type

ERC20

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Aug 30, 2023
code423n4 added a commit that referenced this issue Aug 30, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #928

@c4-pre-sort
Copy link

bytes032 marked the issue as not a duplicate

@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #1455

@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #1782

@c4-pre-sort
Copy link

bytes032 marked the issue as not a duplicate

@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #1782

@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 11, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as unsatisfactory:
Out of scope

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1782 sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants