Unpad memos (#1381)

firoorg · Dec 21, 2023 · 243e6d0 · 243e6d0
1 parent 5e75ba0
commit 243e6d0
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 20 deletions.
diff --git a/src/libspark/coin.cpp b/src/libspark/coin.cpp
@@ -52,6 +52,9 @@ Coin::Coin(
 	std::vector<unsigned char> padded_memo(memo_bytes);
 	padded_memo.resize(this->params->get_memo_bytes());
 
+	// Prepend the unpadded memo length
+	padded_memo.insert(padded_memo.begin(), (unsigned char) memo.size());
+
 	//
 	// Type-specific elements
 	//
@@ -63,7 +66,7 @@ Coin::Coin(
 		MintCoinRecipientData r;
 		r.d = address.get_d();
 		r.k = k;
-		r.memo = std::string(padded_memo.begin(), padded_memo.end());
+		r.padded_memo = std::string(padded_memo.begin(), padded_memo.end());
 		CDataStream r_stream(SER_NETWORK, PROTOCOL_VERSION);
 		r_stream << r;
 		this->r_ = AEAD::encrypt(address.get_Q1()*SparkUtils::hash_k(k), "Mint coin data", r_stream);
@@ -73,7 +76,7 @@ Coin::Coin(
 		r.v = v;
 		r.d = address.get_d();
 		r.k = k;
-		r.memo = std::string(padded_memo.begin(), padded_memo.end());
+		r.padded_memo = std::string(padded_memo.begin(), padded_memo.end());
 		CDataStream r_stream(SER_NETWORK, PROTOCOL_VERSION);
 		r_stream << r;
 		this->r_ = AEAD::encrypt(address.get_Q1()*SparkUtils::hash_k(k), "Spend coin data", r_stream);
@@ -131,10 +134,16 @@ IdentifiedCoinData Coin::identify(const IncomingViewKey& incoming_view_key) {
 			throw std::runtime_error("Unable to identify coin");
 		}
 
+		// Check that the memo length is valid
+		unsigned char memo_length = r.padded_memo[0];
+		if (memo_length > this->params->get_memo_bytes()) {
+			throw std::runtime_error("Unable to identify coin");
+		}
+
 		data.d = r.d;
 		data.v = this->v;
 		data.k = r.k;
-		data.memo = r.memo;
+		data.memo = std::string(r.padded_memo.begin() + 1, r.padded_memo.begin() + 1 + memo_length); // remove the encoded length and padding
 	} else {
 		SpendCoinRecipientData r;
 
@@ -146,10 +155,16 @@ IdentifiedCoinData Coin::identify(const IncomingViewKey& incoming_view_key) {
 			throw std::runtime_error("Unable to identify coin");
 		}
 
+		// Check that the memo length is valid
+		unsigned char memo_length = r.padded_memo[0];
+		if (memo_length > this->params->get_memo_bytes()) {
+			throw std::runtime_error("Unable to identify coin");
+		}
+
 		data.d = r.d;
 		data.v = r.v;
 		data.k = r.k;
-		data.memo = r.memo;
+		data.memo = std::string(r.padded_memo.begin() + 1, r.padded_memo.begin() + 1 + memo_length); // remove the encoded length and padding
 	}
 
 	// Validate the coin

diff --git a/src/libspark/coin.h b/src/libspark/coin.h
@@ -33,15 +33,15 @@ struct RecoveredCoinData {
 struct MintCoinRecipientData {
 	std::vector<unsigned char> d; // encrypted diversifier
 	Scalar k; // nonce
-	std::string memo; // memo
+	std::string padded_memo; // padded memo with prepended one-byte length
 
 	ADD_SERIALIZE_METHODS;
 
 	template <typename Stream, typename Operation>
     inline void SerializationOp(Stream& s, Operation ser_action) {
         READWRITE(d);
 		READWRITE(k);
-		READWRITE(memo);
+		READWRITE(padded_memo);
     }
 };
 
@@ -50,7 +50,7 @@ struct SpendCoinRecipientData {
 	uint64_t v; // value
 	std::vector<unsigned char> d; // encrypted diversifier
 	Scalar k; // nonce
-	std::string memo; // memo
+	std::string padded_memo; // padded memo with prepended one-byte length
 
 	ADD_SERIALIZE_METHODS;
 
@@ -59,7 +59,7 @@ struct SpendCoinRecipientData {
         READWRITE(v);
         READWRITE(d);
 		READWRITE(k);
-		READWRITE(memo);
+		READWRITE(padded_memo);
     }
 };
 
@@ -119,14 +119,14 @@ class Coin {
 
 		// Encrypted coin data is always of a fixed size that depends on coin type
 		// Its tag and key commitment sizes are enforced during its deserialization
-		// For mint coins: encrypted diversifier (with size), encoded nonce, padded memo (with size)
-		// For spend coins: encoded value, encrypted diversifier (with size), encoded nonce, padded memo (with size)
+		// For mint coins: encrypted diversifier (with size), encoded nonce, padded memo (with size), unpadded memo length
+		// For spend coins: encoded value, encrypted diversifier (with size), encoded nonce, padded memo (with size), unpadded memo length
 		READWRITE(r_);
-		if (type == COIN_TYPE_MINT && r_.ciphertext.size() != (1 + AES_BLOCKSIZE) + SCALAR_ENCODING + (1 + params->get_memo_bytes())) {
+		if (type == COIN_TYPE_MINT && r_.ciphertext.size() != (1 + AES_BLOCKSIZE) + SCALAR_ENCODING + (1 + params->get_memo_bytes() + 1)) {
 			std::cout << "Data size " << r_.ciphertext.size() << " but expected " << (AES_BLOCKSIZE + SCALAR_ENCODING + params->get_memo_bytes()) << std::endl;
 			throw std::invalid_argument("Cannot deserialize mint coin due to bad encrypted data");
 		}
-		if (type == COIN_TYPE_SPEND && r_.ciphertext.size() != 8 + (1 + AES_BLOCKSIZE) + SCALAR_ENCODING + (1 + params->get_memo_bytes())) {
+		if (type == COIN_TYPE_SPEND && r_.ciphertext.size() != 8 + (1 + AES_BLOCKSIZE) + SCALAR_ENCODING + (1 + params->get_memo_bytes() + 1)) {
 			std::cout << "Data size " << r_.ciphertext.size() << " but expected " << (8 + AES_BLOCKSIZE + SCALAR_ENCODING + params->get_memo_bytes()) << std::endl;
 			throw std::invalid_argument("Cannot deserialize spend coin due to bad encrypted data");
 		}

diff --git a/src/libspark/params.cpp b/src/libspark/params.cpp
@@ -17,7 +17,7 @@ Params const* Params::get_default() {
             return instance.get();
         }
 
-        std::size_t memo_bytes = 32;
+        std::size_t memo_bytes = 31; // 32 bytes after length prepending!
         std::size_t max_M_range = 16;
         std::size_t n_grootle = 8;
         std::size_t m_grootle = 5;
@@ -37,7 +37,7 @@ Params const* Params::get_test() {
             return instance.get();
         }
 
-        std::size_t memo_bytes = 32;
+        std::size_t memo_bytes = 31; // 32 bytes after length prepending!
         std::size_t max_M_range = 16;
         std::size_t n_grootle = 2;
         std::size_t m_grootle = 4;

diff --git a/src/libspark/params.h b/src/libspark/params.h
@@ -50,7 +50,7 @@ class Params {
     GroupElement U;
 
     // Coin parameters
-    std::size_t memo_bytes;
+    std::size_t memo_bytes; // This MUST NOT exceed 256, since the length is encoded to 8 bits
 
     // Range proof parameters
     std::size_t max_M_range;

diff --git a/src/libspark/test/coin_test.cpp b/src/libspark/test/coin_test.cpp
@@ -28,7 +28,8 @@ BOOST_AUTO_TEST_CASE(mint_identify_recover)
 
     const uint64_t i = 12345;
     const uint64_t v = 86;
-    const std::string memo = "Spam and eggs";
+    const std::string memo = "Spam and eggs are a tasty dish!"; // maximum length
+    BOOST_CHECK_EQUAL(memo.size(), params->get_memo_bytes());
 
     // Generate keys
     SpendKey spend_key(params);
@@ -57,8 +58,8 @@ BOOST_AUTO_TEST_CASE(mint_identify_recover)
     BOOST_CHECK_EQUAL_COLLECTIONS(i_data.d.begin(), i_data.d.end(), address.get_d().begin(), address.get_d().end());
     BOOST_CHECK_EQUAL(i_data.v, v);
     BOOST_CHECK_EQUAL(i_data.k, k);
-    BOOST_CHECK_EQUAL(strcmp(memo.c_str(), i_data.memo.c_str()), 0); // compare strings in a lexicographical manner, as we pad the memo in the coin
-    BOOST_CHECK_EQUAL(i_data.memo.size(), params->get_memo_bytes()); // check that it is padded
+    BOOST_CHECK_EQUAL(i_data.memo, memo);
+
     // Recover coin
     RecoveredCoinData r_data = coin.recover(full_view_key, i_data);
     BOOST_CHECK_EQUAL(
@@ -105,8 +106,7 @@ BOOST_AUTO_TEST_CASE(spend_identify_recover)
     BOOST_CHECK_EQUAL_COLLECTIONS(i_data.d.begin(), i_data.d.end(), address.get_d().begin(), address.get_d().end());
     BOOST_CHECK_EQUAL(i_data.v, v);
     BOOST_CHECK_EQUAL(i_data.k, k);
-    BOOST_CHECK_EQUAL(strcmp(memo.c_str(), i_data.memo.c_str()), 0); // compare strings in a lexicographical manner, as we pad the memo in the coin
-    BOOST_CHECK_EQUAL(i_data.memo.size(), params->get_memo_bytes()); // check that it is padded
+    BOOST_CHECK_EQUAL(i_data.memo, memo);
 
     // Recover coin
     RecoveredCoinData r_data = coin.recover(full_view_key, i_data);