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

The output amount validation in Vault.liquidate() is not correct. #387

Closed
code423n4 opened this issue Jul 14, 2023 · 3 comments
Closed

The output amount validation in Vault.liquidate() is not correct. #387

code423n4 opened this issue Jul 14, 2023 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-427 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/tree/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L566-L568

Vulnerability details

Impact

The output amount validation is not correct in Vault.liquidate(), so the method might accept invalid output amount and refuse valid output amount.

Proof of Concept

In Vault.liquidate(), there is a validation about the output share amount should be less than or equal to the liquidatable yield.

    uint256 _liquidableYield = _liquidatableBalanceOf(_tokenOut);
    if (_amountOut > _liquidableYield) 
      revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield); 

The liquidatable yield amount is in underlying asset token. So the comparison between the share amount and the underlying asset amount is not appropriate.

We could get share tokens from asset tokens via exchange rate. The vault gets _liquidableYield and mints _amountOut, so the correct asset amount equivalent to _amountOut of the share token will be _amountOut * exchange rate. The correct validation should use the asset amount and the current implementation is not correct when the exchange rate is not 1.

Tools Used

Manual Review

Recommended Mitigation Steps

We should use the underlying equivalent with the exchange rate for the validation.

Assessed type

Error

@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 Jul 14, 2023
code423n4 added a commit that referenced this issue Jul 14, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #427

@c4-judge
Copy link
Contributor

c4-judge commented Aug 5, 2023

Picodes marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Aug 5, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Aug 5, 2023

Picodes changed the severity to 3 (High Risk)

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-427 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

2 participants