sherlock-audit / 2023-11-convergence-judging

8 stars 8 forks source link

0x52 - Bribe collection from sdtBlackHole is first-mover takes all which causes loss for multiple gauges that share the same bribe token #179

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

0x52

high

Bribe collection from sdtBlackHole is first-mover takes all which causes loss for multiple gauges that share the same bribe token

Summary

When collecting bribes from sdtBlackHole, the entire balance of the bribe token is sent to the sdtRewardReceiver. This means that shared bribe tokens will credit the entire balance of the bribe token to the first buffer that claims regardless of how much was earned by each underlying token.

Vulnerability Detail

When sdtBuffer is processing rewards, it pulls bribes from sdtBlackHole via the pullSdStakingBribes function in L90.

SdtBlackHole.sol#L96-L134

function pullSdStakingBribes(
    address _processor,
    uint256 _processorRewardsPercentage
) external returns (ICommonStruct.TokenAmount[] memory) {

    ...

    for (uint256 i; i < bribeTokens.length; ) {
        IERC20 bribeToken = bribeTokens[i];

      **@audit this sends the entire balance of the token**

        uint256 balance = bribeToken.balanceOf(address(this));

        if (balance != 0) {
            uint256 claimerRewards = (balance * _processorRewardsPercentage) / 100_000;
            if (claimerRewards > 0) {
                bribeToken.transfer(_processor, claimerRewards);
                balance -= claimerRewards;
            }

            bribeToken.transfer(sdtRewardReceiver, balance);

            _bribeTokensAmounts[i] = ICommonStruct.TokenAmount({token: bribeTokens[i], amount: balance});
        }
        unchecked {
            ++i;
        }
    }
    return _bribeTokensAmounts;
}

When sending the bribe tokens, sdtBlackHole uses the entire balance for sending and crediting the tokens. This creates an issue for bribe tokens that are shared (FXS, CRV, USDC, WETH, etc.). When multiple gauges are earning the same tokens from bribes then the first gauge that calls after the bribes are deposited will receive all of them, which doesn't properly distribute them to the gauges that actually earned them.

Impact

Shared bribe tokens will be first-move takes all which steals rewards from other gauges

Code Snippet

SdtBlackHole.sol#L96-L134

Tool used

Manual Review

Recommendation

Bribes should be sent directly to the buffer instead of to the black hole. Alternatively this could be fixed by adding sub accounts to the blackhole or by making a separate black hole for each asset.

shalbe-cvg commented 11 months ago

Hello,

Thanks a lot for your attention.

After an in-depth review, we have to consider your issue as Invalid. This is the design, the Core team will manage the bribe tokens associated to their buffers.

Regards, Convergence Team

IAm0x52 commented 10 months ago

Escalate.

Bribes for StakeDAO tokens are set up by third parties not by the core team. Admin can only assign tokens to specific buffers. This means that if two tokens are both receiving USDC bribes then only one token will actually receive the bribe. This is entirely unfair to one of the tokens and could represent a significant amount of yield loss for the token whose bribes are being sent to the stakers of another token.

sherlock-admin2 commented 10 months ago

Escalate.

Bribes for StakeDAO tokens are set up by third parties not by the core team. Admin can only assign tokens to specific buffers. This means that if two tokens are both receiving USDC bribes then only one token will actually receive the bribe. This is entirely unfair to one of the tokens and could represent a significant amount of yield loss for the token whose bribes are being sent to the stakers of another token.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 10 months ago

@shalbe-cvg @walk-on-me @0xR3vert

Any comments for the above mentioned point, if not I think I agree with @IAm0x52 unless convergence have no plans to accept same bribe token for buffers.

10xhash commented 10 months ago

Not 100% sure, but it was mentioned by a stakedao member (https://discord.com/channels/802495248563044372/802495248563044375/1178341444545675324) that the bribes would always be distributed in sdTokens if delegated to stakeDao, in line with what is written in technical-docs of convergence

detectiveking123 commented 10 months ago

Yes, bribe tokens are always sdTokens

My issue here: https://github.com/sherlock-audit/2023-11-convergence-judging/issues/195 was invalidated by sponsor for a similar reason

nevillehuang commented 10 months ago

@detectiveking123 There may be some misunderstanding here, the sponsor is mentioning the bribe tokens will never be SDT not saying that other tokens cannot be bribe tokens,

shalbe-cvg commented 10 months ago

Hello,

To clarify, bribe tokens will always be sdTokens and never will be SDT or any other usual tokens like USDC or WETH. Furthermore, every staking contract will have their own bribe rewards and they will be unique. Meaning that a bribe reward token (sdToken) cannot be the same for two different staking contracts.

Regards, Convergence Team

Czar102 commented 10 months ago

From my understanding, this means that this is a non-issue and the judgment should remain as is. cc @IAm0x52

Czar102 commented 10 months ago

Result: Invalid Unique

sherlock-admin2 commented 10 months ago

Escalations have been resolved successfully!

Escalation status: