bin2chen - If WithdrawAddrEnabled = false, execute_claim() will fail

[sherlock-admin3]

bin2chen

Medium

If WithdrawAddrEnabled = false, execute_claim() will fail

Summary

Currently, contracts that execute execute_claim() set DistributionMsg::SetWithdrawAddress first.
If WithdrawAddrEnabled = false, the execution will not succeed and the claim will not be executed.

Vulnerability Detail

Currently the contract executes claims rewards by setting DistributionMsg::SetWithdrawAddress first.

fn execute_claim(
    ctx: ExecuteContext,
    validator: Option<Addr>,
    recipient: Option<AndrAddr>,
) -> Result<Response, ContractError> {
...
    let res = Response::new()
@>      .add_message(DistributionMsg::SetWithdrawAddress {
            address: recipient.to_string(),
        })
        .add_message(DistributionMsg::WithdrawDelegatorReward {
            validator: validator.to_string(),
        })
        .add_attribute("action", "validator-claim-reward")
        .add_attribute("recipient", recipient)
        .add_attribute("validator", validator.to_string());

    Ok(res)
}

If the configuration WithdrawAddrEnabled is changed to false, setting DistributionMsg::SetWithdrawAddress will fail!
This will prevent the execution of the claim
https://github.com/cosmos/cosmos-sdk/tree/main/x/distribution#msgsetwithdrawaddress

MsgSetWithdrawAddress

By default, the withdraw address is the delegator address. To change its withdraw address, a delegator must send a MsgSetWithdrawAddress message. Changing the withdraw address is possible only if the parameter WithdrawAddrEnabled is set to true.

func (k Keeper) SetWithdrawAddr(ctx context.Context, delegatorAddr sdk.AccAddress, withdrawAddr sdk.AccAddress) error
 if k.blockedAddrs[withdrawAddr.String()] {
  fail with "`{withdrawAddr}` is not allowed to receive external funds"
 }

 if !k.GetWithdrawAddrEnabled(ctx) {
  fail with `ErrSetWithdrawAddrDisabled`
 }

 k.SetDelegatorWithdrawAddr(ctx, delegatorAddr, withdrawAddr)

Impact

can't claim reward

Code Snippet

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

Tool used

Manual Review

Recommendation

when set DistributionMsg::SetWithdrawAddress , SubMsg using ReplyOn.Error, which is ignored when this message returns an error, to avoid the whole execute_claim from failing!

[neko-nyaa]

Escalate

#15, #67, #68 are dupes of this issue. This family of issues shows various impacts when withdrawAddrEnabled is set to false, and the root cause of the revert is the message MsgSetWithdrawAddress failing.

[sherlock-admin3]

Escalate

#15, #67, #68 are dupes of this issue. This family of issues shows various impacts when withdrawAddrEnabled is set to false, and the root cause of the revert is the message MsgSetWithdrawAddress failing.

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.

[cvetanovv]

I agree with the escalation.

The root cause of the issue is when withdrawAddrEnabled is set to false. That's why I plan to duplicate all similar issues related to WithdrawAddrEnabled = false.

I am planning to accept the escalation and duplicate #15, #67, #68, and #52 with this issue.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• neko-nyaa: accepted

[cowboy0015]

withdraw_fund function is now working as an emergency for withdrawing funds (including locked funds due to unexpected cases)

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
andromedaprotocol/andromeda-core#559

[bin2chen66]

fix-reviews note:
andromedaprotocol/andromeda-core#559
This PR canceled the execution of SetWithdrawAddress.
After the reward is triggered, get back all the balance in the contract via execute_withdraw_fund()
Fixed this issue