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

VoterProxy incorrectly assumes a 1-1 mapping between the gauge and the LP tokens. #51

Open
code423n4 opened this issue May 31, 2022 · 5 comments
Labels
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 question Further information is requested sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L270
https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L140

Vulnerability details

Impact

When calling withdrawAll, to compute the amount to withdraw, the contract checks the balance of gauge tokens and assume that 1 gauge token = 1 LP token by doing uint256 amount = balanceOfPool(_gauge).add(IERC20(_token).balanceOf(address(this)));. Overall this assumption may not hold and this would lead to a loss of funds when calling withdrawAll.

Proof of concept

Indeed this is false in some cases, check for example https://etherscan.io/address/0x3785Ce82be62a342052b9E5431e9D3a839cfB581 where the total supply is not the same as the balance of LP tokens held by the contract. You can also check the contract code where you can see there is a staking_factor between the balance and the underlying LP token balance.

Mitigation steps

Use the total supply of pool.token which is a better proxy to know how much to withdraw when withdrawing all.

@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 May 31, 2022
code423n4 added a commit that referenced this issue May 31, 2022
@jetbrain10 jetbrain10 added the question Further information is requested label Jun 15, 2022
@jetbrain10
Copy link
Collaborator

are you refering we need to calc the staking _factor by ourself?

@GalloDaSballo
Copy link
Collaborator

@jetbrain10 the warden says that some Deposits will not return the same amount of Lp Tokens.

See this example from the contract linked by the Warden:
https://etherscan.io/tx/0xd3eab573697d4fb92ebe4d91d35b03795d384ac45f7a723b321c98f6da2420cb

@GalloDaSballo
Copy link
Collaborator

I think this means the contracts may break when integrating with Angle Protocol.

AfaI'm aware CRV, Balancer will return a 1-1 between the Deposit Token and the Gauge Token

@GalloDaSballo
Copy link
Collaborator

The warden has shown evidence of the GaugeLP-Token being "rebased" from the "usual" 1-1 to a ratio (assuming due to cost / increasing in value in underlying).

Because:

  • The Sponsor system is meant to integrate with multiple protocols
  • The warden has shown a specific example (ANGLE) of the math bring broken

Considering that this is contingent on the sponsor launching an integration with the Angle Protocol, using Gauges that "rebase", I believe the finding to be Valid and of Medium Severity

Most likely remediation will require poking the gauge for exchange rates and also checking available tokens after withdrawing

@liveactionllama
Copy link

Per discussion with @jetbrain10 (sponsor), adding sponsor acknowledged label to this issue.

@liveactionllama liveactionllama added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 question Further information is requested 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

4 participants