search

sherlock-audit / 2024-05-andromeda-ado-judging

1 stars 0 forks source link

J4X_ - Attacker can freeze users first rewards #50

Open sherlock-admin4 opened 5 months ago

sherlock-admin4 commented 5 months ago

J4X_

High

Attacker can freeze users first rewards

Summary

The andromeda-validator-staking contract has a vulnerability related to the staking rewards withdrawal process. If the withdrawal address is not set correctly, rewards can be unintentionally distributed to the contract itself, causing them to become stuck. This can be exploited by an attacker who can front-run the owner's first claim transaction and cause the rewards to be irretrievably sent to the contract. The impact of this issue is the loss of all rewards accrued before un-bonding.

Vulnerability Detail

The andromeda-validator-staking allows the owner to stake tokens to a chosen validator. The delegation will then generate staking rewards. To allow the contract owner to withdraw these rewards, the execute_claim() function is implemented. To be able to claim the tokens correctly, two messages have to be sent:

  1. DistributionMsg::SetWithdrawAddress - sets the address to withdraw to the recipients address
  2. DistributionMsg::WithdrawDelegatorReward - withdraws the rewards

If the first message is not sent, the withdrawal address is set to the delegator which in our case is the andromeda-validator-staking contract. When the owner calls the execute_claim() function directly, this leads to no issues, as the two functions are called correctly.

The issues occur as there are multiple other scenarios why rewards will be distributed besides the direct call via DistributionMsg::WithdrawDelegatorReward. Rewards will be distributed if a user's stake increases. The other option is that an un-bonding occurs, in which case rewards are also distributed. In total there are four scenarios why rewards will be distributed without a call to DistributionMsg::WithdrawDelegatorReward:

  1. Owner stakes or un-stakes
  2. Validator is jailed/tombstoned
  3. The validator leaves the set willingly
  4. Attacker stakes on behalf of the owner (which works as execute_stake() is not restricted)

For this case, we will only consider 2., 3., and 4. as 1. would require some owner wrongdoing. If one of these cases occurs before the owner has claimed rewards for the first time, the rewards will be sent directly to the andromeda-validator-staking contract. The tokens will become stuck there as the contract does not implement a way to retrieve/re-stake funds.

For the fourth scenario, a malicious attacker can intentionally abuse this and wait until the owner tries to call execute_claim() for the first time. When he sees the tx, he front-runs it and stakes 1 token on behalf of the owner, which will result in the owner's rewards getting sent to the andromeda-validator-staking contract and getting stuck. As the SetWithdrawAddress message will only be sent afterward, the recipient is still the andromeda-validator-staking contract.

Impact

The issue results in all rewards accruing before the un-bonding getting stuck in the contract and being effectively lost. As the andromeda-validator-staking contract does not implement a migrate() function, the funds can not be rescued by upgrading the contract.

Code Snippet

https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/bbbf73e5d1e4092ab42ce1f827e33759308d3786/andromeda-core/contracts/finance/andromeda-validator-staking/src/contract.rs#L186-L242

Tool used

Manual Review

Recommendation

We recommend mitigating this issue by setting a withdrawal_address when calling instantiate(). This withdrawal address should then be set on each call to execute_stake(), execute_unstake(), and execute_withdraw_fund(). This way, tokens can never be lost due to an unset withdrawal address.

gjaldon commented 5 months ago

Escalate

This issue is invalid because the following statement is incorrect.

The tokens will become stuck there as the contract does not implement a way to retrieve/re-stake funds.

There is functionality for the owner to withdraw funds from the Staking contract through AndromedaMsg::Withdraw. Below are the links to the relevant code that shows how the AndromedaMsg::Withdraw message is handled.

There is, however, a separate and unrelated issue in execute_withdraw() reported here that prevents withdrawal of funds. The loss of funds is only possible if there is no Withdrawal implementation or if the withdrawal implementation does not work. The report incorrectly states there is no withdrawal functionality and does not mention any issue with withdrawal.

sherlock-admin3 commented 5 months ago

Escalate

This issue is invalid because the following statement is incorrect.

The tokens will become stuck there as the contract does not implement a way to retrieve/re-stake funds.

There is functionality for the owner to withdraw funds from the Staking contract through AndromedaMsg::Withdraw. Below are the links to the relevant code that shows how the AndromedaMsg::Withdraw message is handled.

There is, however, a separate and unrelated issue in execute_withdraw() reported here that prevents withdrawal of funds. The loss of funds is only possible if there is no Withdrawal implementation or if the withdrawal implementation does not work. The report incorrectly states there is no withdrawal functionality and does not mention any issue with withdrawal.

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.

J4X-98 commented 5 months ago

Hey @gjaldon,

your escalation is based on you stating that the sentence "The tokens will become stuck there as the contract does not implement a way to retrieve/re-stake funds." is wrong. A few sentences below, you link your own issue that describes withdrawals as not being possible due to a missing implementation.

So, effectively, the issue is valid, and the mitigation works and mitigates the problem. The only critique you have is that you would have liked me to go more into detail on why withdrawals do not work. I can't see in any way how this would lead to the issue being invalidated, but I'm looking forward to hearing the judge's opinion.

gjaldon commented 4 months ago

Hello @J4X-98,

In my opinion, the finding is incomplete because it assumes that there is no way to withdraw funds even though the functionality exists. This issue would also be addressed by a fix in withdrawing of funds.

The issue is only valid if there is no way to withdraw funds, since there would be no impact if funds are withdrawable. This issue is only valid in combination with the issue I reported. Since it fails to identify the other root cause that would lead to loss of funds, I think this is invalid.

J4X-98 commented 4 months ago

Hey,

The functionality of both contracts is clearly intended to distribute rewards to a preset claimant address, not let them be sent to the contract. The code clearly tries to set a withdraw address before each time rewards are distributed. This can also be seen in the vesting functionality. However the developer forgot this for some cases in the staking module.

Withdrawing these rewards, which were incorrectly distributed to the contract, via an additional function, would be a emergency rescue, but not the intended behavior. Additionally this does not work, so there is no way to rescue these tokens, which is exactly what I described in the issue.

You're escalation is based on me not describing (the not working) emergency fix in as much detail as you would like to, which does in no way invalidate the issue. The issue is that the withdraw address is not correctly set at the start, which would lead to the intended behavior.

cvetanovv commented 4 months ago

After all, as far as I understand, the rewards can't be rescued and are lost. But I don't think it is High severity. The losses are not significant, and settling the withdrawal address will fix the problem.

The rule for High:

Definite loss of funds without (extensive) limitations of external conditions.

I plan to reject the escalation to invalidate the issue, but I'll make it Medium severity.

J4X-98 commented 4 months ago

Hey @cvetanovv ,

Setting the withdrawal address will only protect from the future loss of rewards but will not recover the originally lost funds. This will clearly lead to a loss of fund which would classify this as a high.

gjaldon commented 4 months ago

@cvetanovv the rewards can't be rescued and are lost only because the WithdrawFunds functionality is not working. My report describes the issue of funds not being withdrawable but it is not mentioned here. Instead, it makes an incorrect statement:

The tokens will become stuck there as the contract does not implement a way to retrieve/re-stake funds.

The contract does implement a way to retrieve funds but it just so happens that it isn't working which is the point of the issue I reported.

Should a report be valid if its described impact (loss of funds in this case) is only possible combined with another bug that the reporter and report failed to identify? The impact is only possible when combined with a bug reported by someone else.

J4X-98 commented 4 months ago

As commented before by me as an answer to you stating the same claim as now, I stated that withdrawal is not possible. You are trying to invalidate this issue based on the reason that I don't go into enough detail on why withdrawals do not work. This in now way contradicts the original issue and is your personal preference.

cvetanovv commented 4 months ago

My decision remains the same. To reject the escalation and downgrade the severity to Medium. This issue and its duplicates show how it is possible to have a loss of rewards. But because the loss is limited and the withdrawal address setting can stop the loss, I believe it is Medium severity.

WangSecurity commented 4 months ago

Result: Medium Has duplicates

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

cowboy0015 commented 3 months ago

withdraw_funds function is now working as an emergency to handle frozen rewards in the contract.

cowboy0015 commented 3 months ago

Auto distribution did not happen for the jailed validator in e2e test. Going to upload the video demonstrating the testing process