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

Anyone Can Arbitrarily Mint Fungible Tokens In VaderPoolV2.mintFungible() #147

Closed
code423n4 opened this issue Nov 15, 2021 · 3 comments
Closed
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") VaderPoolV2

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

The mintFungible() function is callable by any user that wishes to mint liquidity pool fungible tokens. The protocol expects a user to first approve the contract as a spender before calling mintFungible(). However, any arbitrary user could monitor the blockchain for contract approvals that match VaderPoolV2.sol and effectively frontrun their call to mintFungible() by setting the to argument to their own address. As a result, the nativeDeposit and foreignDeposit amounts are transferred from the victim, and LP tokens are minted and finally transferred to the malicious user who is represented by the to address.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L284-L335

Tools Used

Manual code review.
Discussions with dev.

Recommended Mitigation Steps

Consider removing the from argument in mintFungible() and update the safeTransferFrom() calls to instead transfer from msg.sender.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 15, 2021
code423n4 added a commit that referenced this issue Nov 15, 2021
@SamSteinGG SamSteinGG added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 25, 2021
@SamSteinGG
Copy link
Collaborator

The pool contracts, similarly to Uniswap V2, are never meant to be interacted with directly.

@alcueca
Copy link
Collaborator

alcueca commented Dec 11, 2021

You need to enforce that somehow.

@SamSteinGG
Copy link
Collaborator

Upon second consideration, the functions relating to the minting of synths and wrapped tokens should have had the onlyRouter modifier and thus are indeed vulnerable. Issue accepted.

@SamSteinGG SamSteinGG added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Dec 16, 2021
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 sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") VaderPoolV2
Projects
None yet
Development

No branches or pull requests

4 participants