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

Curve Strategy Yield can be Lost by Griefing due to Delta Balance Check #1429

Open
code423n4 opened this issue Aug 4, 2023 · 10 comments
Open
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 grade-b M-11 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report 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

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoNativeStrategy.sol#L151-L156

Vulnerability details

Impact

TricryptoLPStrategy-compound computes the amount of CRV to Sell as:
uint256 crvAmount = crvBalanceAfter - crvBalanceBefore;

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoNativeStrategy.sol#L151-L156

    function compound(bytes memory) public {
        uint256 claimable = lpGauge.claimable_tokens(address(this));
        if (claimable > 0) {
            uint256 crvBalanceBefore = rewardToken.balanceOf(address(this));
            minter.mint(address(lpGauge));
            uint256 crvBalanceAfter = rewardToken.balanceOf(address(this));

            if (crvBalanceAfter > crvBalanceBefore) {
                uint256 crvAmount = crvBalanceAfter - crvBalanceBefore;

This assumes that minter.mint(address(lpGauge)); will cause tokens to be sent to the Strategy

However, a griefer could call claim_rewards(STRATEGY): to cause the CRV to be sent directly to it before a call to compound is made.

This breaks the check (since it will result in a 0)

And causes total Loss of Yield

POC

  • Attacker calls claim_rewards(STRATEGY)
  • The Strategy no longer compounds the rewards

Code from Curve Gauge

https://arbiscan.io/address/0x555766f3da968ecbefa690ffd49a2ac02f47aa5f#code#L544

@external
@nonreentrant('lock')
def claim_rewards(_addr: address = msg.sender, _receiver: address = ZERO_ADDRESS):
    """
    @notice Claim available reward tokens for `_addr`
    @param _addr Address to claim for
    @param _receiver Address to transfer rewards to - if set to
                     ZERO_ADDRESS, uses the default reward receiver
                     for the caller
    """
    if _receiver != ZERO_ADDRESS:
        assert _addr == msg.sender  # dev: cannot redirect when claiming for another user
    self._checkpoint_rewards(_addr, self.totalSupply, True, _receiver)

As you can see the gauge allows claiming on behalf, which will break the delta check from the Strategy

Mitigation

Use the absolute value for Curve or similar reward tokens, also consider adding a sweep function while protecting the deposit tokens

Assessed type

ERC4626

@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 Aug 4, 2023
code423n4 added a commit that referenced this issue Aug 4, 2023
@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Aug 7, 2023
@c4-pre-sort
Copy link

minhquanym marked the issue as primary issue

@minhquanym
Copy link
Member

Similar #269

@c4-sponsor
Copy link

cryptotechmaker (sponsor) confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 6, 2023
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 30, 2023
@c4-judge
Copy link

dmvt changed the severity to QA (Quality Assurance)

@c4-judge
Copy link

dmvt marked the issue as grade-b

@GalloDaSballo
Copy link

The finding states:

  • The delta balance can be griefed by claiming on the behalf of the strategy, causing a loss to depositors
  • This finding is logically the same as 805, on a different set of contracts and as a different integration
  • In contrast to the AAVE finding, claiming CRV rewards is permissioneless, anyone can grief the yield

@c4-judge
Copy link

c4-judge commented Oct 8, 2023

This previously downgraded issue has been upgraded by dmvt

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Oct 8, 2023
@dmvt
Copy link

dmvt commented Oct 8, 2023

Agreed. I don't remember exactly why I downgraded this one in the first place. May have been a mistaken button press on my part. Thanks for raising it.

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 8, 2023
@c4-judge
Copy link

c4-judge commented Oct 8, 2023

dmvt marked the issue as selected for report

@dmvt
Copy link

dmvt commented Oct 8, 2023

Note I'm leaving this as a unique because of the third point

  • In contrast to the AAVE finding, claiming CRV rewards is permissioneless, anyone can grief the yield

@C4-Staff C4-Staff added the M-11 label Oct 10, 2023
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 grade-b M-11 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report 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

8 participants