diff --git a/contracts/remittance_nft/src/lib.rs b/contracts/remittance_nft/src/lib.rs index 5e1b1df0..897bed1c 100644 --- a/contracts/remittance_nft/src/lib.rs +++ b/contracts/remittance_nft/src/lib.rs @@ -1258,7 +1258,13 @@ impl RemittanceNFT { return Vec::new(&env); } let mut page = Vec::new(&env); - let end = (offset + limit).min(len); + // `offset + limit` is not safe here: contract Wasm is built with + // `overflow-checks = true` (contracts/Cargo.toml), so any offset above + // zero combined with a large limit (e.g. offset = 1, limit = u32::MAX) + // traps the whole call. Saturate first, THEN clamp to `len`, so a + // saturated u32::MAX still resolves to the real available tail instead + // of being used as an oversized range bound. + let end = offset.saturating_add(limit).min(len); for idx in offset..end { page.push_back(history.get(idx).unwrap()); } diff --git a/contracts/remittance_nft/src/test.rs b/contracts/remittance_nft/src/test.rs index bc4902a6..991b903f 100644 --- a/contracts/remittance_nft/src/test.rs +++ b/contracts/remittance_nft/src/test.rs @@ -3074,3 +3074,112 @@ fn test_lifecycle_methods_resume_after_unpause() { client.burn(&second_user, &None); assert!(client.get_metadata(&second_user).is_none()); } + +#[test] +fn test_get_score_history_limit_u32_max_does_not_trap() { + // Regression test for #1144: `get_score_history` computed + // `let end = (offset + limit).min(len);`. With `overflow-checks = true` in + // the release profile (contracts/Cargo.toml) a plain `+` traps the whole + // call whenever `offset + limit` exceeds u32::MAX, e.g. limit = u32::MAX + // with offset = 1. The saturating value must still be clamped down to the + // real available length, so valid offsets keep returning the tail. + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let user = Address::generate(&env); + + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + client.mint( + &user, + &500, + &create_test_hash(&env, 31), + &create_test_uri(&env), + &create_test_commitment(&env, 1), + &None, + ); + + // 5 history entries, ledgers 1..=5. + for sequence in 1..=5u32 { + env.ledger().set_sequence_number(sequence); + client.update_score(&user, &1_000_000_000, &None); + } + + // offset = 0: `0 + u32::MAX` does not overflow by itself, but the saturated + // end must still be clamped to `len` rather than used as a range bound. + let all = client.get_score_history(&user, &0, &u32::MAX); + assert_eq!(all.len(), 5); + assert_eq!(all.get(0).unwrap().ledger, 1); + assert_eq!(all.get(4).unwrap().ledger, 5); + + // offset = 1: `1 + u32::MAX` overflows u32 and trapped before this fix. + // It must return the remaining 4 entries, in order. + let tail = client.get_score_history(&user, &1, &u32::MAX); + assert_eq!(tail.len(), 4); + assert_eq!(tail.get(0).unwrap().ledger, 2); + assert_eq!(tail.get(3).unwrap().ledger, 5); + + // The last valid offset still yields exactly one entry. + let last = client.get_score_history(&user, &4, &u32::MAX); + assert_eq!(last.len(), 1); + assert_eq!(last.get(0).unwrap().ledger, 5); + + // offset >= len stays an empty page, even with a saturated limit. + assert_eq!(client.get_score_history(&user, &5, &u32::MAX).len(), 0); +} + +#[test] +fn test_get_score_history_saturating_add_preserves_valid_pagination() { + // #1144 ordering check: `.min(len)` must be applied *after* + // `saturating_add`. Clamping first (or using the saturated value directly + // as the range bound) would run the loop past `len`, and + // `history.get(idx).unwrap()` would panic instead of returning a tail. + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let user = Address::generate(&env); + + let contract_id = env.register(RemittanceNFT, ()); + let client = RemittanceNFTClient::new(&env, &contract_id); + + client.initialize(&admin); + client.mint( + &user, + &500, + &create_test_hash(&env, 32), + &create_test_uri(&env), + &create_test_commitment(&env, 1), + &None, + ); + + // 10 history entries, ledgers 1..=10. + for sequence in 1..=10u32 { + env.ledger().set_sequence_number(sequence); + client.update_score(&user, &1_000_000_000, &None); + } + + // A plain, non-saturating page is unchanged by the fix. + let page = client.get_score_history(&user, &3, &1_000); + assert_eq!(page.len(), 7); + assert_eq!(page.get(0).unwrap().ledger, 4); + assert_eq!(page.get(6).unwrap().ledger, 10); + + // limit = u32::MAX - 1 only overflows once the offset is added; the + // saturated result still clamps to the available tail. + let saturated = client.get_score_history(&user, &3, &(u32::MAX - 1)); + assert_eq!(saturated.len(), 7); + assert_eq!(saturated.get(0).unwrap().ledger, 4); + assert_eq!(saturated.get(6).unwrap().ledger, 10); + + // limit = 0 still returns an empty page (empty range, no underflow). + assert_eq!(client.get_score_history(&user, &3, &0).len(), 0); + + // offset = 0 with a fully saturated limit returns everything available. + let everything = client.get_score_history(&user, &0, &u32::MAX); + assert_eq!(everything.len(), 10); + assert_eq!(everything.get(9).unwrap().ledger, 10); +}