search

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

1 stars 0 forks source link

g - Calculating tax amount does not include taxes in `WasmMsg::Execute` messages #37

Open sherlock-admin3 opened 5 months ago

sherlock-admin3 commented 5 months ago

g

Medium

Calculating tax amount does not include taxes in WasmMsg::Execute messages

Summary

get_tax_amount() fetches the amount of tax to be paid by totaling the amounts in the message transfers generated by the on_funds_transfer() hook. However, it only counts CosmosMsg::Bank(BankMsg::Send) messages and not the WasmMsg::Execute messages. This either fails the transfer or allows payors to skip paying taxes.

Vulnerability Detail

The issue lies in get_tax_amount()'s mapping logic over the messages generated by on_funds_transfer().

ref: std/src/common/rates.rs::get_tax_amount()

pub fn get_tax_amount(
    msgs: &[SubMsg],
    base_amount: Uint128,
    remaining_amount_after_royalties: Uint128,
) -> Uint128 {
    let deducted_amount = base_amount - remaining_amount_after_royalties;
    msgs.iter()
        .map(|msg| {
            if let CosmosMsg::Bank(BankMsg::Send { amount, .. }) = &msg.msg {
                amount[0].amount
            } else {
                Uint128::zero()
            }
        })

Only taxes wrapped in BankMsg::Send are included. However, fee recipients with a msg set will get generated a WasmMsg::Execute message instead.

ref: std/src/amp/recipient.rs::generate_direct_msg()

Ok(match &self.msg {
    Some(message) => SubMsg::new(WasmMsg::Execute {
        contract_addr: resolved_addr.to_string(),
        msg: message.clone(),
        funds,
    }),
    None => SubMsg::new(CosmosMsg::Bank(BankMsg::Send {
        to_address: resolved_addr.to_string(),
        amount: funds,
    })),
})

Any taxes sent to recipients with a msg set will not be counted by get_tax_amount().

Impact

The following effects will take place:

  1. When at least one royalty is wrapped in a WasmMsg::Execute, get_tax_amount() will fail. Payment transfers for non-fungible ADO contracts will fail since non-fungible ADO contracts use get_tax_amont().
  2. When there are no royalties and at least one tax is in a WasmMsg::Execute msg, the tax wrapped in a WasmMsg::Execute message will be ignored. Payers can skip paying the WasmMsg::Execute taxes in these cases.

In effect, fee recipients with msg can not be used or it will lead to failures in non-fungible ADO contracts.

Code Snippet

Tool used

Manual Review

Recommendation

Consider modifying get_tax_amount() to count the taxes in WasmMsg::Execute messages.

gjaldon commented 5 months ago

Escalate

This valid issue shows that not all taxes will be counted. Only taxes wrapped in BankMsg::Send will be counted even though some taxes will be wrapped in WasmMsg::Execute. This causes either valid payment transactions to fail or allows payers to skip paying some taxes.

std/src/common/rates.rs::get_tax_amount() is also in-scope.

sherlock-admin3 commented 5 months ago

Escalate

This valid issue shows that not all taxes will be counted. Only taxes wrapped in BankMsg::Send will be counted even though some taxes will be wrapped in WasmMsg::Execute. This causes either valid payment transactions to fail or allows payers to skip paying some taxes.

std/src/common/rates.rs::get_tax_amount() is also in-scope.

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 commented 4 months ago

Indeed, not all fees are counted when get_tax_amount() is called. So, I plan to accept the escalation and make the issue a valid Medium.

gjaldon commented 4 months ago

Thank you 🙏🏼

WangSecurity commented 4 months ago

@gjaldon @MxAxM are there any duplicates?

WangSecurity commented 4 months ago

Result: Medium Unique

sherlock-admin2 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: