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

Unable to remove liquidity in Recovery Mode #323

Open
code423n4 opened this issue Dec 1, 2021 · 1 comment
Open

Unable to remove liquidity in Recovery Mode #323

code423n4 opened this issue Dec 1, 2021 · 1 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")

Comments

@code423n4
Copy link
Contributor

Handle

gzeon

Vulnerability details

Impact

According to https://github.com/code-423n4/2021-11-malt#high-level-overview-of-the-malt-protocol

When the Malt price TWAP drops below a specified threshold (eg 2% below peg) then the protocol will revert any transaction that tries to remove Malt from the AMM pool (ie buying Malt or removing liquidity). Users wanting to remove liquidity can still do so via the UniswapHandler contract that is whitelisted in recovery mode.

However, in https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L236
liquidity removed is directly sent to msg.sender, which would revert if it is not whitelisted
https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/PoolTransferVerification.sol#L53

Recommended Mitigation Steps

Liquidity should be removed to UniswapHandler contract, then the proceed is sent to msg.sender

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 1, 2021
code423n4 added a commit that referenced this issue Dec 1, 2021
@0xScotch 0xScotch added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Dec 8, 2021
@GalloDaSballo
Copy link
Collaborator

I believe this finding to be correct, because of the whitelisting on verifyTransfer, during recovery mode the removal of liquidity from UniSwapV2Pair will perform safeTransfers: https://github.com/Uniswap/v2-core/blob/4dd59067c76dea4a0e8e4bfdda41877a6b16dedc/contracts/UniswapV2Pair.sol#L148

This means that the _beforeTokenTransfer will be called which eventually will call verifyTransfer which, if the price is below peg will revert.

Transfering the funds to the whitelisted contract should avoid this issue.

I'd like to remind the sponsor that anyone could deploy similar swapping contracts (or different ones such as curve), so if a person is motivate enough, all the whitelisting could technically be sidestepped.

That said, given the condition of LPing on Uniswap, the check and the current system would make it impossible to withdraw funds.

Because this does indeed compromises the availability of funds (effectively requiring the admin to unstock them manually via Whitelisting each user), I agree with High Severity

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")
Projects
None yet
Development

No branches or pull requests

3 participants