Payment transactions succeed even when recipient transfers fail

Summary

generate_msg_cw20() and generate_direct_msg() are Recipient messages that send payment to the recipient. However, the messages are fire-and-forget and do not fail the transaction which causes unintended consequences that will be described in the Impact section.

Vulnerability Detail

All the resulting SubMsgs of generate_msg_cw20() and generate_direct_msg() are created from SubMsg::new().

ref: https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/amp/recipient.rs#L48-L65

    None => SubMsg::new(CosmosMsg::Bank(BankMsg::Send {
        to_address: resolved_addr.to_string(),
        amount: funds,
    })),

SubMsg::new() creates fire-and-forget messages by setting reply_on to never.

pub fn new(msg: impl Into<CosmosMsg<T>>) -> Self {
    SubMsg {
        id: UNUSED_MSG_ID,
        msg: msg.into(),
        reply_on: ReplyOn::Never,
        gas_limit: None,
    }
}

generate_direct_msg() and generate_msg_cw20() are used in the following:

Timelock ADO
Vesting ADO
Splitter ADO
Rates Module
Crowdfund ADO
All contracts that use ADOContract's execution handler (all ADOs)

Impact

The impact can vary depending on the contract. In Timelock ADO, when funds are released, the escrows of the released funds are removed even when the transfers to the fund recipients fail. This is because failure of fund recipient transfers will not revert the transaction.

fn execute_release_funds(
    // ... snip ...
    for key in keys.iter() {
        let funds: Escrow = escrows().load(deps.storage, key.clone())?;
        if !funds.is_locked(&env.block)? {
            let msg = funds
                .recipient
                .generate_direct_msg(&deps.as_ref(), funds.coins)?;
            msgs.push(msg);
            escrows().remove(deps.storage, key.clone())?;
        }
    }

    ensure!(!msgs.is_empty(), ContractError::FundsAreLocked {});

    // @audit-issues uses `add_submessages()` for sync messages but it does not matter since the recipient messages 
    // are to set to never reply.
    Ok(Response::new().add_submessages(msgs).add_attributes(vec![
        attr("action", "release_funds"),
        attr("recipient_addr", recipient_addr),
    ]))
}

Once escrows are removed, those funds are permanently lost and can no longer be claimed by the intended recipients.

Code Snippet

Tool used

Manual Review

Recommendation

Consider setting the reply_on fields to ReplyOn::Always for both generate_direct_msg() and generate_msg_cw20() and only rely on add_messages() to set the messages to fire-and-forget.

sherlock-admin3 commented 3 months ago

Escalate

This report shows an issue that affects all ADO contracts. Payment transfers will succeed even when the required recipient transfers fail. The impact varies but in the case of Timelock ADO, it leads to a loss of funds because the escrow is removed even if the fund transfer failed. In the Rates module, some payments will need to pay for taxes or royalties. However, the taxes and royalties can fail but the transaction will still succeed.

The issue is in generate_direct_msg() and generate_msg_cw20() which are functions in a file in-scope packages/std/src/amp/recipient.rs.

You've deleted an escalation for this issue.

Kallya commented 3 months ago

The message will still revert because submessages with ReplyOn::Never set have the same semantics as a normal message (this can be seen in the CosmWasm documentation here or here).

gjaldon commented 3 months ago

You are right @Kallya! Thank you 🙏🏼

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

g - Payment transactions succeed even when recipient transfers fail #73