Skip to content

Commit

Permalink
Now check that the IBC events are valid with respect to storage changes.
Browse files Browse the repository at this point in the history
  • Loading branch information
@murisi
murisi committed Jun 7, 2024
1 parent 20d113b commit f6a7580
Show file tree
Hide file tree
Showing 7 changed files with 181 additions and 150 deletions.
15 changes: 0 additions & 15 deletions crates/events/src/extend.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,21 +483,6 @@ impl EventAttributeEntry<'static> for Info {
}
}

/// Extend an [`Event`] with `packet_ack` data, indicating the success or
/// failure of processing a received packet.
pub struct PacketAck(pub Vec<u8>);

impl EventAttributeEntry<'static> for PacketAck {
type Value = Vec<u8>;
type ValueOwned = Self::Value;

const KEY: &'static str = "packet_ack";

fn into_value(self) -> Self::Value {
self.0
}
}

/// Extend an [`Event`] with `masp_tx_block_index` data, indicating that the tx
/// at the specified index in the block contains a valid masp transaction.
pub struct MaspTxBlockIndex(pub TxIndex);
Expand Down
1 change: 0 additions & 1 deletion crates/ibc/src/actions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use namada_state::{
StorageResult, StorageWrite, WlState, DB,
};
use namada_token as token;
use token::DenominatedAmount;

use crate::event::IbcEvent;
use crate::{
Expand Down
1 change: 0 additions & 1 deletion crates/ibc/src/context/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,6 @@ pub trait IbcCommonContext: IbcStorageContext {

/// Calculate the packet commitment
fn compute_packet_commitment(
&self,
packet_data: &[u8],
timeout_height: &TimeoutHeight,
timeout_timestamp: &Timestamp,
Expand Down
2 changes: 1 addition & 1 deletion crates/ibc/src/context/token_transfer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use namada_core::ibc::core::handler::types::error::ContextError;
use namada_core::ibc::core::host::types::identifiers::{ChannelId, PortId};
use namada_core::ibc::IBC_ESCROW_ADDRESS;
use namada_core::uint::Uint;
use namada_token::{read_denom, Amount, Denomination};
use namada_token::Amount;

use super::common::IbcCommonContext;
use crate::storage;
Expand Down
270 changes: 153 additions & 117 deletions crates/namada/src/ledger/native_vp/masp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use namada_core::masp::encode_asset_type;
use namada_core::storage::Key;
use namada_gas::GasMetering;
use namada_governance::storage::is_proposal_accepted;
use namada_ibc::event as ibc_events;
use namada_ibc::event::IbcEvent;
use namada_ibc::event::{IbcEvent, PacketAck};
use namada_ibc::{event as ibc_events, IbcCommonContext};
use namada_proof_of_stake::Epoch;
use namada_sdk::masp::{verify_shielded_tx, MaybeIbcAddress};
use namada_state::{ConversionState, OptionExt, ResultExt, StateRead};
Expand All @@ -41,26 +41,23 @@ use token::storage_key::{
};
use token::Amount;

use crate::address::{IBC, InternalAddress};
use crate::address::{InternalAddress, IBC};
use crate::ledger::ibc::storage;
use crate::ledger::ibc::storage::{
ibc_trace_key, ibc_trace_key_prefix, is_ibc_trace_key,
};
use crate::ledger::native_vp;
use crate::ledger::native_vp::ibc::context::VpValidationContext;
use crate::ledger::native_vp::{Ctx, NativeVp};
use crate::sdk::ibc::apps::transfer::types::is_sender_chain_source;
use crate::sdk::ibc::core::channel::types::acknowledgement::AcknowledgementStatus;
use crate::sdk::ibc::core::channel::types::commitment::PacketCommitment;
use crate::token;
use crate::token::MaspDigitPos;
use crate::uint::{Uint, I320};
use crate::vm::WasmCacheAccess;
use crate::ledger::ibc::storage::ibc_token;
use crate::sdk::ibc::is_ibc_denom;
use crate::sdk::ibc::IbcTokenHash;
use crate::ledger::events::extend::PacketAck;
use crate::sdk::ibc::core::channel::types::acknowledgement::AcknowledgementStatus;

/// Packet event types
const SEND_PACKET_EVENT: &str = "send_packet";
const RECEIVE_PACKET_EVENT: &str = "recv_packet";
const WRITE_ACK_EVENT: &str = "write_acknowledgement";

#[allow(missing_docs)]
Expand Down Expand Up @@ -400,13 +397,14 @@ where

let ibc_address = MaybeIbcAddress::Address(IBC);
// Public keys must be the hash of the sources/targets
let ibc_address_hash = TransparentAddress(<[u8; 20]>::from(
ripemd::Ripemd160::digest(sha2::Sha256::digest(
&ibc_address.serialize_to_vec(),
)),
));
let ibc_address_hash =
TransparentAddress(<[u8; 20]>::from(ripemd::Ripemd160::digest(
sha2::Sha256::digest(&ibc_address.serialize_to_vec()),
)));
// Enable decoding the IBC address hash
changed_balances.decoder.insert(ibc_address_hash, ibc_address);
changed_balances
.decoder
.insert(ibc_address_hash, ibc_address);

// Extract the IBC events
let ibc_events: BTreeSet<_> = self
Expand All @@ -425,66 +423,134 @@ where
continue;
};
// Check if this is a Transfer packet
if let Ok(packet_data) =
let Ok(packet_data) =
serde_json::from_slice::<PacketData>(&msg.data)
{
let address =
MaybeIbcAddress::Ibc(packet_data.receiver.to_string());
// Public keys must be the hash of the sources/targets
let address_hash = TransparentAddress(<[u8; 20]>::from(
ripemd::Ripemd160::digest(sha2::Sha256::digest(
&address.serialize_to_vec(),
)),
));
changed_balances.decoder.insert(address_hash, address);
changed_balances
.pre
.entry(address_hash)
.or_insert(ValueSum::zero());
changed_balances
.post
.entry(ibc_address_hash)
.or_insert(ValueSum::zero());
// Also enable the tracking of received IBC
let pre_entry = changed_balances
.pre
.entry(ibc_address_hash)
.or_insert(ValueSum::zero());
let post_entry = changed_balances
.post
.entry(address_hash)
.or_insert(ValueSum::zero());

match ibc_event.kind().sub_domain() {
// This event is emitted on the sender
SEND_PACKET_EVENT => {
// Since IBC denominations are derived from Addresses
// when sending, we have to do a reverse look-up of the
// relevant token Address
let token = changed_balances
.ibc_denoms
.get(&packet_data.token.denom.to_string())
.cloned()
else {
continue;
};

// Get the packet commitment from post-storage that corresponds
// to this event
let address =
MaybeIbcAddress::Ibc(packet_data.receiver.to_string());
// Public keys must be the hash of the sources/targets
let address_hash = TransparentAddress(<[u8; 20]>::from(
ripemd::Ripemd160::digest(sha2::Sha256::digest(
&address.serialize_to_vec(),
)),
));
changed_balances.decoder.insert(address_hash, address);
changed_balances
.pre
.entry(address_hash)
.or_insert(ValueSum::zero());
changed_balances
.post
.entry(ibc_address_hash)
.or_insert(ValueSum::zero());
// Required for the IBC internal Address to release funds
let pre_entry = changed_balances
.pre
.entry(ibc_address_hash)
.or_insert(ValueSum::zero());
// Required for the packet's receiver to get funds
let post_entry = changed_balances
.post
.entry(address_hash)
.or_insert(ValueSum::zero());

match ibc_event.kind().sub_domain() {
// This event is emitted on the sender
SEND_PACKET_EVENT => {
// Ensure that the event corresponds to the current changes
// to storage
let commitment_key = storage::commitment_key(
&msg.port_id_on_a,
&msg.chan_id_on_a,
msg.seq_on_a,
);
if !keys_changed.contains(&commitment_key) {
// Otherwise ignore this event
continue;
}
let Some(storage_commitment): Option<PacketCommitment> =
self.ctx
.read_bytes_post(&commitment_key)?
.map(Into::into)
else {
// Ignore this event if it does not exist
continue;
};
// Check that the packet commitment in storage is the same
// as that derived from this event
let packet_commitment = VpValidationContext::<'a, 'a, S, CA>::compute_packet_commitment(
&msg.data,
&msg.timeout_height_on_b,
&msg.timeout_timestamp_on_b,
);
if packet_commitment != storage_commitment {
// Ignore this event if the commitments are not equal
continue;
}
// Since IBC denominations are derived from Addresses
// when sending, we have to do a reverse look-up of the
// relevant token Address
let token = changed_balances
.ibc_denoms
.get(&packet_data.token.denom.to_string())
.cloned()
// If the reverse lookup failed, then guess the Address
// that might have yielded the IBC denom. However,
// guessing an IBC token address cannot possibly be
// correct due to the structure of query_ibc_denom
.or_else(|| Address::decode(&packet_data.token.denom.to_string())
.ok()
.filter(|x| !matches!(x, Address::Internal(InternalAddress::IbcToken(_)))))
.ok_or_err_msg("Unable to decode IBC token")?;
let delta = ValueSum::from_pair(
token.clone(),
Amount::from_uint(Uint(*packet_data.token.amount), 0)
.unwrap(),
);
// Enable funds to be received by the receiver in the
// IBC packet
*post_entry = checked!(post_entry + &delta)
.map_err(native_vp::Error::new)?
},
// This event is emitted on the receiver
RECEIVE_PACKET_EVENT => {
.or_else(|| {
Address::decode(packet_data.token.denom.to_string())
.ok()
.filter(|x| {
!matches!(
x,
Address::Internal(
InternalAddress::IbcToken(_)
)
)
})
})
.ok_or_err_msg("Unable to decode IBC token")?;
let delta = ValueSum::from_pair(
token.clone(),
Amount::from_uint(Uint(*packet_data.token.amount), 0)
.unwrap(),
);
// Enable funds to be received by the receiver in the
// IBC packet
*post_entry = checked!(post_entry + &delta)
.map_err(native_vp::Error::new)?
}
// This event is emitted on the receiver
WRITE_ACK_EVENT => {
// Ensure that the event corresponds to the current changes
// to storage
let ack_key = storage::ack_key(
&msg.port_id_on_a,
&msg.chan_id_on_a,
msg.seq_on_a,
);
if !keys_changed.contains(&ack_key) {
// Otherwise ignore this event
continue;
}
let acknowledgement = ibc_event
.raw_read_attribute::<PacketAck<'_>>()
.ok_or_err_msg(
"packet_ack attribute missing from \
write_acknowledgement",
)?;
let acknowledgement: AcknowledgementStatus =
serde_json::from_slice(acknowledgement.as_ref())
.wrap_err("Decoding acknowledment failed")?;
// If the transfer was a failure, then enable funds to
// be withdrawn from the IBC internal address
if acknowledgement.is_successful() {
// Mirror how the IBC token is derived in
// gen_ibc_shielded_transfer in the non-refund case
let ibc_denom = self.query_ibc_denom(
Expand All @@ -497,56 +563,25 @@ where
&msg.chan_id_on_a,
&msg.port_id_on_b,
&msg.chan_id_on_b,
).into_storage_result()
.map_err(Error::NativeVpError)?;
)
.into_storage_result()
.map_err(Error::NativeVpError)?;
let delta = ValueSum::from_pair(
token.clone(),
Amount::from_uint(Uint(*packet_data.token.amount), 0)
.unwrap(),
Amount::from_uint(
Uint(*packet_data.token.amount),
0,
)
.unwrap(),
);
// Enable funds to be taken from the IBC internal
// address and be deposited elsewhere
*pre_entry = checked!(pre_entry + &delta)
.map_err(native_vp::Error::new)?
},
// This event is emitted on the sender
WRITE_ACK_EVENT => {
let acknowledgement = ibc_event
.raw_read_attribute::<PacketAck>()
.ok_or_err_msg("packet_ack attribute missing from write_acknowledgement")?;
let acknowledgement: AcknowledgementStatus =
serde_json::from_slice(acknowledgement.as_ref())
.wrap_err("Decoding acknowledment failed")?;
// If the transfer was a failure, then enable funds to
// be withdrawn from the IBC internal address
if !acknowledgement.is_successful() {
// Mirror how the IBC token is derived in
// gen_ibc_shielded_transfer in the refund case
let ibc_denom = self.query_ibc_denom(
&packet_data.token.denom.to_string(),
Some(&Address::Internal(InternalAddress::Ibc)),
)?;
let token = if ibc_denom.contains('/') {
ibc_token(ibc_denom)
} else {
// the token is a base token
Address::decode(&ibc_denom)
.wrap_err("Invalid token")?
};
let delta = ValueSum::from_pair(
token.clone(),
Amount::from_uint(Uint(*packet_data.token.amount), 0)
.unwrap(),
);
// Enable funds to be withdrawn from the IBC
// internal address
*pre_entry = checked!(pre_entry + &delta)
.map_err(native_vp::Error::new)?
}
},
// Ignore all other IBC events
_ => {},
.map_err(native_vp::Error::new)?;
}
}
// Ignore all other IBC events
_ => {}
}
}

Expand Down Expand Up @@ -636,8 +671,9 @@ where

// Ensure that this transaction is authorized by all involved parties
for signer in signers {
if let Some(MaybeIbcAddress::Address(Address::Internal(InternalAddress::Ibc))) =
changed_balances.decoder.get(&signer)
if let Some(MaybeIbcAddress::Address(Address::Internal(
InternalAddress::Ibc,
))) = changed_balances.decoder.get(&signer)
{
// If the IBC address is a signatory, then it means that either
// Tx - Transaction(s) causes a decrease in the IBC balance or
Expand All @@ -660,7 +696,7 @@ where
"Simultaneous credit and debit of IBC account \
in a MASP transaction not allowed",
)
.into();
.into();
tracing::debug!("{error}");
return Err(error);
}
Expand Down
Loading

0 comments on commit f6a7580

Please sign in to comment.