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

Badger rewards from Hidden Hand can permanently prevent Strategy from receiving bribes #111

Open
code423n4 opened this issue Jun 18, 2022 · 4 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 sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L428-L430
https://github.com/Badger-Finance/badger-vaults-1.5/blob/3c96bd83e9400671256b235422f63644f1ae3d2a/contracts/BaseStrategy.sol#L351
https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L407-L408

Vulnerability details

Impact

If the contract receives rewards from the hidden hand marketplace in BADGER then the contract tries to transfer the same amount of tokens twice to two different accounts, once with _sendBadgerToTree() in MyStrategy and again with _processExtraToken() in the BasicStrategy contract. As it is very likely that the strategy will not start with any BADGER tokens, the second transfer will revert (as we are using safeTransfer). This means that claimBribesFromHiddenHand() will always revert preventing any other bribes from being received.

Proof of Concept

  1. claimBribesFromHiddenHand() is called by strategist
  2. Multiple bribes are sent to the strategy including BADGER. For example lets say 50 USDT And 50 BADGER
  3. Strategy receives BADGER and calls _handleRewardTransfer() which calls _sendBadgerToTree(). 50 BADGER is sent to the Badger Tree so balance has dropped to 0.
  4. 50 Badger is then again sent to Vault however balance is 0 so the command fails and reverts
  5. No more tokens can be claimed anymore

Tools Used

VS Code

Recommended Mitigation Steps

_processExtraToken() eventually sends the badger to the badger tree through the Vault contract. Change

    function _sendBadgerToTree(uint256 amount) internal {
        IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
        _processExtraToken(address(BADGER), amount);
    }

to

    function _sendBadgerToTree(uint256 amount) internal {
        _processExtraToken(address(BADGER), amount);
    }
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jun 18, 2022
code423n4 added a commit that referenced this issue Jun 18, 2022
@GalloDaSballo
Copy link
Collaborator

Developer oversight yeah

@GalloDaSballo GalloDaSballo added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 19, 2022
@shuklaayush
Copy link
Collaborator

Yeah, badger bribes can't be claimed. Not sure if I'll call it high risk but definitely an oversight

@KenzoAgada
Copy link

Duplicate of #2

@jack-the-pug jack-the-pug added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value valid and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jul 3, 2022
@jack-the-pug jack-the-pug added valid and removed valid labels Jul 11, 2022
@GalloDaSballo
Copy link
Collaborator

We mitigated by fixing the mistake

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 sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") valid
Projects
None yet
Development

No branches or pull requests

5 participants