diff --git a/CHANGELOG.md b/CHANGELOG.md index 463a59daf..23a999f29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - fix: Kernel Send Msg vulnerability [(#915)](https://github.com/andromedaprotocol/andromeda-core/pull/915) - fix: Duplicate Redeem in Exchange [(#919)](https://github.com/andromedaprotocol/andromeda-core/pull/919) - fix: LP Denom Extraction in Osmosis Socket [(#951)](https://github.com/andromedaprotocol/andromeda-core/pull/951) +- fix: Add expiration to timelock threshold [(#1009)](https://github.com/andromedaprotocol/andromeda-core/pull/1009) ## Release 4 diff --git a/Cargo.lock b/Cargo.lock index be3bfe542..e15570a11 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1066,7 +1066,7 @@ dependencies = [ [[package]] name = "andromeda-timelock" -version = "2.1.1-b.7" +version = "2.1.1-b.8" dependencies = [ "andromeda-app", "andromeda-finance", diff --git a/contracts/finance/andromeda-timelock/Cargo.toml b/contracts/finance/andromeda-timelock/Cargo.toml index aa36c4479..382969114 100644 --- a/contracts/finance/andromeda-timelock/Cargo.toml +++ b/contracts/finance/andromeda-timelock/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "andromeda-timelock" -version = "2.1.1-b.7" +version = "2.1.1-b.8" edition = "2021" rust-version = "1.86.0" diff --git a/contracts/finance/andromeda-timelock/src/contract.rs b/contracts/finance/andromeda-timelock/src/contract.rs index 7a11dbf94..381aaa3a9 100644 --- a/contracts/finance/andromeda-timelock/src/contract.rs +++ b/contracts/finance/andromeda-timelock/src/contract.rs @@ -16,6 +16,7 @@ use cosmwasm_std::{ }; use crate::state::{escrows, get_key, get_keys_for_recipient}; +use andromeda_finance::timelock::EscrowCondition; // version info for migration info const CONTRACT_NAME: &str = "crates.io:andromeda-timelock"; @@ -110,6 +111,39 @@ fn execute_hold_funds( ])) } +/// Determines who should receive the funds based on the escrow condition and current block state +fn determine_release_recipient( + escrow: &Escrow, + block: &cosmwasm_std::BlockInfo, + key: &[u8], +) -> Result<(Recipient, String), ContractError> { + if let Some(EscrowCondition::MinimumFunds { expiration, .. }) = &escrow.condition { + if expiration.is_expired(block) { + // Expiration reached before minimum funds - return to original sender + // The key format is [owner.as_bytes(), recipient.as_bytes()].concat() + // We need to extract the owner part. Since we know the recipient address, + // we can find where it starts in the key. + let recipient_bytes = escrow.recipient_addr.as_bytes(); + if key.len() > recipient_bytes.len() { + let owner_end = key.len() - recipient_bytes.len(); + if &key[owner_end..] == recipient_bytes { + let owner_bytes = &key[..owner_end]; + if let Ok(owner_str) = std::str::from_utf8(owner_bytes) { + return Ok(( + Recipient::from_string(owner_str.to_string()), + "expired_before_minimum".to_string(), + )); + } + } + } + // Fallback if key parsing fails - return to recipient + return Ok((escrow.recipient.clone(), "expired_fallback".to_string())); + } + } + // Default case: send to intended recipient + Ok((escrow.recipient.clone(), "condition_met".to_string())) +} + fn execute_release_funds( ctx: ExecuteContext, recipient_addr: Option, @@ -129,9 +163,11 @@ fn execute_release_funds( 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)?; + // Determine who should receive the funds based on the condition + let (recipient_for_funds, _release_reason) = + determine_release_recipient(&funds, &env.block, key)?; + + let msg = recipient_for_funds.generate_direct_msg(&deps.as_ref(), funds.coins)?; msgs.push(msg); escrows().remove(deps.storage, key.clone())?; } @@ -155,24 +191,25 @@ fn execute_release_specific_funds( } = ctx; let recipient = recipient.unwrap_or_else(|| info.sender.to_string()); let key = get_key(&owner, &recipient); - let escrow = escrows().may_load(deps.storage, key.clone())?; - match escrow { - None => Err(ContractError::NoLockedFunds {}), - Some(escrow) => { - ensure!( - !escrow.is_locked(&env.block)?, - ContractError::FundsAreLocked {} - ); - escrows().remove(deps.storage, key)?; - let msg = escrow - .recipient - .generate_direct_msg(&deps.as_ref(), escrow.coins)?; - Ok(Response::new().add_submessage(msg).add_attributes(vec![ - attr("action", "release_funds"), - attr("recipient_addr", recipient), - ])) - } - } + let escrow = escrows() + .load(deps.storage, key.clone()) + .map_err(|_err| ContractError::NoLockedFunds {})?; + + ensure!( + !escrow.is_locked(&env.block)?, + ContractError::FundsAreLocked {} + ); + escrows().remove(deps.storage, key.clone())?; + + // Determine who should receive the funds based on the condition + let (recipient_for_funds, _release_reason) = + determine_release_recipient(&escrow, &env.block, &key)?; + + let msg = recipient_for_funds.generate_direct_msg(&deps.as_ref(), escrow.coins)?; + Ok(Response::new().add_submessage(msg).add_attributes(vec![ + attr("action", "release_funds"), + attr("recipient_addr", recipient), + ])) } #[cfg_attr(not(feature = "library"), entry_point)] diff --git a/contracts/finance/andromeda-timelock/src/testing/tests.rs b/contracts/finance/andromeda-timelock/src/testing/tests.rs index 07b995da1..b65acd596 100644 --- a/contracts/finance/andromeda-timelock/src/testing/tests.rs +++ b/contracts/finance/andromeda-timelock/src/testing/tests.rs @@ -276,10 +276,12 @@ fn test_execute_release_funds_min_funds_condition() { let info = message_info(&Addr::unchecked(OWNER), &[coin(100, "uusd")]); let msg = ExecuteMsg::HoldFunds { - condition: Some(EscrowConditionInput::MinimumFunds(vec![ - coin(200, "uusd"), - coin(100, "uluna"), - ])), + condition: Some(EscrowConditionInput::MinimumFunds { + funds: vec![coin(200, "uusd"), coin(100, "uluna")], + expiration: Expiry::AtTime(Milliseconds::from_seconds( + env.block.time.seconds() + 86400, // 1 day expiration + )), + }), recipient: None, }; let _res = execute(deps.as_mut(), env.clone(), info.clone(), msg).unwrap(); @@ -422,10 +424,12 @@ fn test_execute_release_specific_funds_min_funds_condition() { let info = message_info(&Addr::unchecked(OWNER), &[coin(100, "uusd")]); let msg = ExecuteMsg::HoldFunds { - condition: Some(EscrowConditionInput::MinimumFunds(vec![ - coin(200, "uusd"), - coin(100, "uluna"), - ])), + condition: Some(EscrowConditionInput::MinimumFunds { + funds: vec![coin(200, "uusd"), coin(100, "uluna")], + expiration: Expiry::AtTime(Milliseconds::from_seconds( + env.block.time.seconds() + 86400, // 1 day expiration + )), + }), recipient: None, }; let _res = execute(deps.as_mut(), env.clone(), info.clone(), msg).unwrap(); @@ -472,29 +476,78 @@ fn test_execute_release_specific_funds_min_funds_condition() { ); } -// #[test] -// fn test_execute_receive() { -// let mut deps = mock_dependencies_custom(&[]); -// let env = mock_env(); -// let owner = "owner"; -// let funds = vec![Coin::new(1000, "uusd")]; -// let info = message_info(&Addr::unchecked(owner), &funds); - -// let msg_struct = ExecuteMsg::HoldFunds { -// condition: None, -// recipient: None, -// }; -// let msg_string = encode_binary(&msg_struct).unwrap(); - -// let msg = ExecuteMsg::Receive(Some(msg_string)); - -// let received = execute(deps.as_mut(), env, info.clone(), msg).unwrap(); -// let expected = Response::default().add_attributes(vec![ -// attr("action", "hold_funds"), -// attr("sender", info.sender.to_string()), -// attr("recipient", "Addr(\"owner\")"), -// attr("condition", "None"), -// ]); - -// assert_eq!(expected, received) -// } +#[test] +fn test_minimum_funds_now_has_mandatory_expiration() { + let mut deps = mock_dependencies_custom(&[]); + init(&mut deps); + let mut env = mock_env(); + + // User deposits 100 uusd with minimum threshold that includes mandatory expiration + let info = message_info(&Addr::unchecked(OWNER), &[coin(100, "uusd")]); + let msg = ExecuteMsg::HoldFunds { + condition: Some(EscrowConditionInput::MinimumFunds { + funds: vec![ + coin(1000, "uusd"), // Requires 1000 uusd total + coin(500, "uluna"), // Requires 500 uluna total + ], + expiration: Expiry::AtTime(Milliseconds::from_seconds( + env.block.time.seconds() + 86400, // 1 day expiration + )), + }), + recipient: None, + }; + let _res = execute(deps.as_mut(), env.clone(), info.clone(), msg).unwrap(); + + // Verify funds are locked initially (expected behavior) + let msg = ExecuteMsg::ReleaseFunds { + recipient_addr: None, + start_after: None, + limit: None, + }; + let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()); + assert_eq!(ContractError::FundsAreLocked {}, res.unwrap_err()); + + // SOLUTION: Now MinimumFunds has mandatory expiration - no more permanent locking! + + // Scenario 1: User tries to add more funds but still can't reach the threshold + let info_partial = message_info(&Addr::unchecked(OWNER), &[coin(400, "uusd")]); + let msg_partial = ExecuteMsg::HoldFunds { + condition: None, // No new condition, keeps original MinimumFunds condition + recipient: None, + }; + let _res = execute(deps.as_mut(), env.clone(), info_partial, msg_partial).unwrap(); + + // Still locked because we only have 500 uusd total, need 1000 uusd + 500 uluna + let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()); + assert_eq!(ContractError::FundsAreLocked {}, res.unwrap_err()); + + // SOLUTION: Fast forward past expiration - funds are no longer permanently locked! + env.block.time = Timestamp::from_seconds(env.block.time.seconds() + 86401); + + // Now funds can be released back to the original sender + let res = execute(deps.as_mut(), env.clone(), info.clone(), msg).unwrap(); + + // Funds are returned to OWNER (original sender) since threshold was never met + let bank_msg = BankMsg::Send { + to_address: OWNER.into(), + amount: vec![coin(500, "uusd")], // 100 + 400 returned to sender + }; + let expected_res: Response = Response::new() + .add_message(bank_msg) + .add_attribute("action", "release_funds") + .add_attribute("recipient_addr", OWNER); + assert_response( + &res, + &expected_res, + "minimum_funds_with_expiration_recovery", + ); + + // Verify the funds are no longer in escrow + let query_msg = QueryMsg::GetLockedFunds { + owner: OWNER.to_string(), + recipient: OWNER.to_string(), + }; + let res = query(deps.as_ref(), env, query_msg).unwrap(); + let val: GetLockedFundsResponse = from_json(res).unwrap(); + assert!(val.funds.is_none()); // No funds locked anymore - problem solved! +} diff --git a/packages/andromeda-finance/src/timelock.rs b/packages/andromeda-finance/src/timelock.rs index a89048621..e7fe491fd 100644 --- a/packages/andromeda-finance/src/timelock.rs +++ b/packages/andromeda-finance/src/timelock.rs @@ -12,8 +12,12 @@ use cosmwasm_std::{ensure, Api, BlockInfo, Coin}; pub enum EscrowConditionInput { /// Requires a given time Expiration(Expiry), - /// Requires a minimum amount of funds to be deposited. - MinimumFunds(Vec), + /// Requires a minimum amount of funds to be deposited before expiration. + /// If expiration is reached before minimum funds are met, funds are returned to senders. + MinimumFunds { + funds: Vec, + expiration: Expiry, + }, } impl EscrowConditionInput { @@ -24,7 +28,12 @@ impl EscrowConditionInput { EscrowConditionInput::Expiration(expiry) => { EscrowCondition::Expiration(expiry.get_time(block)) } - EscrowConditionInput::MinimumFunds(funds) => EscrowCondition::MinimumFunds(funds), + EscrowConditionInput::MinimumFunds { funds, expiration } => { + EscrowCondition::MinimumFunds { + funds, + expiration: expiration.get_time(block), + } + } } } } @@ -34,8 +43,12 @@ impl EscrowConditionInput { pub enum EscrowCondition { /// Requires a given time Expiration(MillisecondsExpiration), - /// Requires a minimum amount of funds to be deposited. - MinimumFunds(Vec), + /// Requires a minimum amount of funds to be deposited before expiration. + /// If expiration is reached before minimum funds are met, funds are returned to senders. + MinimumFunds { + funds: Vec, + expiration: MillisecondsExpiration, + }, } #[cw_serde] @@ -69,7 +82,11 @@ impl Escrow { ContractError::InvalidAddress {} ); - if let Some(EscrowCondition::MinimumFunds(funds)) = &self.condition { + if let Some(EscrowCondition::MinimumFunds { + funds, + expiration: _, + }) = &self.condition + { ensure!( !funds.is_empty(), ContractError::InvalidFunds { @@ -84,10 +101,7 @@ impl Escrow { ContractError::DuplicateCoinDenoms {} ); } - // Explicitly stop here as it is alright if the Escrow is unlocked in this case, ie, - // the intially deposited funds are greater or equal to the minimum imposed by this - // condition. - return Ok(()); + // Continue to expiration validation below for MinimumFunds } ensure!( @@ -103,7 +117,12 @@ impl Escrow { None => Ok(false), Some(condition) => match condition { EscrowCondition::Expiration(expiration) => Ok(!expiration.is_expired(block)), - EscrowCondition::MinimumFunds(funds) => { + EscrowCondition::MinimumFunds { funds, expiration } => { + // If expiration is reached, funds should be unlocked (returned to senders) + if expiration.is_expired(block) { + return Ok(false); + } + // Otherwise, check if minimum funds are met Ok(!self.min_funds_deposited(funds.clone())) } }, @@ -284,10 +303,10 @@ mod tests { let valid_escrow = Escrow { recipient: recipient.clone(), coins: vec![coin(100, "uluna")], - condition: Some(EscrowCondition::MinimumFunds(vec![ - coin(100, "uusd"), - coin(100, "uluna"), - ])), + condition: Some(EscrowCondition::MinimumFunds { + funds: vec![coin(100, "uusd"), coin(100, "uluna")], + expiration: MillisecondsExpiration::from_seconds(5000), + }), recipient_addr: OWNER.to_string(), }; let block = BlockInfo { @@ -301,7 +320,10 @@ mod tests { let valid_escrow = Escrow { recipient: recipient.clone(), coins: vec![coin(200, "uluna")], - condition: Some(EscrowCondition::MinimumFunds(vec![coin(100, "uluna")])), + condition: Some(EscrowCondition::MinimumFunds { + funds: vec![coin(100, "uluna")], + expiration: MillisecondsExpiration::from_seconds(5000), + }), recipient_addr: OWNER.to_string(), }; valid_escrow.validate(deps.as_ref().api, &block).unwrap(); @@ -310,7 +332,10 @@ mod tests { let invalid_escrow = Escrow { recipient: recipient.clone(), coins: vec![coin(100, "uluna")], - condition: Some(EscrowCondition::MinimumFunds(vec![])), + condition: Some(EscrowCondition::MinimumFunds { + funds: vec![], + expiration: MillisecondsExpiration::from_seconds(5000), + }), recipient_addr: OWNER.to_string(), }; assert_eq!( @@ -326,11 +351,10 @@ mod tests { let invalid_escrow = Escrow { recipient, coins: vec![coin(100, "uluna")], - condition: Some(EscrowCondition::MinimumFunds(vec![ - coin(100, "uusd"), - coin(100, "uluna"), - coin(200, "uusd"), - ])), + condition: Some(EscrowCondition::MinimumFunds { + funds: vec![coin(100, "uusd"), coin(100, "uluna"), coin(200, "uusd")], + expiration: MillisecondsExpiration::from_seconds(5000), + }), recipient_addr: OWNER.to_string(), }; assert_eq!( diff --git a/tests/timelock.rs b/tests/timelock.rs index d367c35b0..a8c552aea 100644 --- a/tests/timelock.rs +++ b/tests/timelock.rs @@ -192,7 +192,12 @@ fn test_timelock() { // Test Case 3: Minimum Funds - let escrow_condition = EscrowConditionInput::MinimumFunds(vec![coin(1000, "uandr")]); + let escrow_condition = EscrowConditionInput::MinimumFunds { + funds: vec![coin(1000, "uandr")], + expiration: Expiry::AtTime(Milliseconds::from_seconds( + router.block_info().time.seconds() + 86400, // 1 day expiration + )), + }; timelock .execute_hold_funds( &mut router, @@ -218,7 +223,12 @@ fn test_timelock() { assert_eq!(err, ContractError::FundsAreLocked {}); - let escrow_condition = EscrowConditionInput::MinimumFunds(vec![coin(1000, "uandr")]); + let escrow_condition = EscrowConditionInput::MinimumFunds { + funds: vec![coin(1000, "uandr")], + expiration: Expiry::AtTime(Milliseconds::from_seconds( + router.block_info().time.seconds() + 86400, // 1 day expiration + )), + }; timelock .execute_hold_funds( &mut router,