Comprehensive Code Review - Addressing Bugs, Errors, and Potential Security Issues

[Adefebrian]

Description: Hey team! 🚀 After diving deep into the codebase, I’ve identified a series of bugs, potential errors, and some areas that could open us up to security risks. These issues span across multiple files, including the ChainServer, Config, and Wallet classes. Let’s break down what I found and how we can tighten things up:

1. File Existence Check in save() Method

• What's up? The save() method in the Wallet class checks if a file exists using std::filesystem::exists(path), which is great, but doesn’t handle race conditions. Another process could create the file between the check and the file creation.
• Potential Issue: Race condition could lead to unexpected behavior or errors.
• Solution: Implement a retry mechanism or use atomic operations to handle race conditions safely.

2. Exception Handling in open_wallet()

• What's up? The open_wallet() function currently catches std::runtime_error and nlohmann::detail::parse_error. However, exceptions like std::bad_alloc or std::ios_base::failure could also occur and are not caught.
• Potential Issue: Uncaught exceptions could crash the application or cause undefined behavior.
• Solution: Add a catch-all handler (catch (const std::exception& e)) to gracefully handle unexpected exceptions.

3. Inconsistent Error Messages

• What's up? Error messages in the open_wallet() and save() methods are inconsistent in style and tone, including the use of emojis like 🙂, which might not be suitable for all contexts.
• Potential Issue: Inconsistent messaging can confuse users and appear unprofessional.
• Solution: Standardize error messages across the codebase for a consistent tone and style.

4. Logical Errors in Wallet Constructor

• What's up? The Wallet constructor checks if the public key and address match the private key, throwing a generic std::runtime_error with the message "Inconsistent data" if they don’t.
• Potential Issue: The error message is too vague, making debugging difficult.
• Solution: Provide detailed error messages that specify which data is inconsistent, and log the expected versus actual values for better debugging.

5. Exception-Safety in process() Function

• What's up? The process() function has complex conditional logic and throws exceptions for error handling, but doesn’t use RAII (Resource Acquisition Is Initialization) techniques.
• Potential Issue: Lack of RAII can lead to resource leaks if an exception is thrown.
• Solution: Refactor the code to use RAII or ensure that all resources are properly released even if an exception occurs.

6. Incomplete Initialization Check

• What's up? In the process() function, if neither ai.create_given nor ai.restore_given is true, the code attempts to open an existing wallet file without checking if ai.file_arg is valid.
• Potential Issue: If ai.file_arg is null or empty, this could lead to crashes or undefined behavior.
• Solution: Add a validation check to ensure ai.file_arg is non-null and valid before attempting to open the wallet file.

7. Potentially Unsafe auto Type in read_amount()

• What's up? The read_amount() function uses auto for the balance_lambda variable. While convenient, it could lead to unexpected types if the lambda’s return type changes.
• Potential Issue: Subtle bugs due to unexpected types.
• Solution: Explicitly define the type of balance_lambda to avoid type-related issues.

8. Potential Buffer Overflow in read_with_msg()

• What's up? The read_with_msg() function uses cin >> input; for user input, which could overflow if the input exceeds the buffer size.
• Potential Issue: Buffer overflow vulnerabilities.
• Solution: Switch to std::getline(cin, input); to safely handle inputs of varying lengths.

9. Lack of Command-Line Arguments Validation

• What's up? The process() function assumes that command-line arguments are correctly validated by the cmdline_parser function.
• Potential Issue: Mistakes in argument parsing could lead to undefined behavior.
• Solution: Implement additional validation checks for command-line arguments within the process() function.

10. Undefined Behavior with CompactUInt::compact()

• What's up? The CompactUInt::compact() method is called with a list-initialized Funds object, but it’s unclear if this method can handle list initialization.
• Potential Issue: Undefined behavior if compact() isn’t designed for list initialization.
• Solution: Ensure that CompactUInt::compact() correctly handles list initialization or refactor the code to avoid it.

11. Inconsistent assert() Usage

• What's up? The process() function uses assert() to validate a signature, but assert() may be disabled in production, skipping this check.
• Potential Issue: Critical checks could be skipped in production.
• Solution: Replace assert() with runtime checks that are always executed, even in production.

12. Incomplete Error Handling in endpoint.send_transaction()

• What's up? The return value of endpoint.send_transaction() is destructured into code and error, but there’s no handling if code is non-zero but error is empty.
• Potential Issue: Misalignment between code and error could lead to unclear error handling.
• Solution: Add checks to ensure code and error are handled correctly, providing clear feedback on what went wrong.

---

Steps to Fix:

1. File Existence Check: Implement atomic operations or retry mechanisms in save() to handle race conditions.
2. Exception Handling: Add a catch-all handler in open_wallet() to catch unexpected exceptions.
3. Standardize Error Messages: Update error messages across the codebase for consistency.
4. Wallet Constructor: Improve error messaging with more detail and logging.
5. RAII Implementation: Refactor the process() function to use RAII or ensure proper resource cleanup.
6. Initialization Check: Validate ai.file_arg before using it in the process() function.
7. Type Safety: Define the type of balance_lambda explicitly in read_amount().
8. Input Safety: Use std::getline in read_with_msg() to prevent buffer overflows.
9. Argument Validation: Add additional validation checks for command-line arguments in process().
10. CompactUInt Handling: Ensure CompactUInt::compact() correctly handles list initialization.
11. Assert Replacement: Replace assert() with runtime checks in process().
12. Error Handling: Improve error handling in endpoint.send_transaction().

---

Environment:

• OS: macOS M1

---

Let’s get these issues ironed out to make our codebase stronger, safer, and more reliable. 💪 If you have any questions or need further details, feel free to reach out!

[ByPumbaa]

Thank you:

1. There is no atomic way to block file creation or deletion across different file systems. The check is sufficiently good. Furthermore it does not make sense to do repeated checks: When the file does not exist and we are good to write why check again? And even if we checked again the problem that a new file could be created is still present between the last check and the file write. So both your solutions a) atomicity and b) repeated check are not possible or do not make sense.
2. Yes we can change that.
3. Please specify the line and file of messages you like to change.
4. We can write inconsistent wallet data. This will only be triggered if the wallet file is changed by someone and there is no point detailing which consistency check failed.
5. You are wrong, every object created is RAII itself, on exceptions the stack unwinds and every destructor is called. No leak possible. If you don't agree please specify which variable you think leaks memory on exception.
6. The ai argument for the process() function is filled by an auto generated function (read about https://www.gnu.org/software/gengetopt/gengetopt.html). This is proven software and the output should be valid.
7. No, the balance_lambda is a lambda variable. You cannot specify a type, every lambda has its own implicit unique type. Your comment does not make sense.
8. This is wrong, no overflow is possible in operator>>(std::istream&, std::string&).
9. See 6. It is fine to assume that command-line arguments are correctly validated by the auto-generated argument parser because gengetopt is proven software.
10. You are wrong saying " list-initialized Funds", brace initialization is used. You say "but it’s unclear if this method can handle list initialization.". It is clear from the function declaration. No list initialization used. No issue here. We can remove the braces to avoid an unnecessary temporary object (which is actually optimized away anyway).
11. The assert is good and should be there. We use assert where we assume that something holds and to crash the program when we were wrong. This should be a crash and not a user reported error to reflect the fact that this should NEVER happen.
12. Nothing is "destructed into ..." We use structured binding to report both, error code and error message as returned by the node. When there is an empty error message we print an empty error message. I see no problem here. We just report what we got from the node API.

Please make it clear when you use AI and post rather on Discord than opening an issue. Furthermore, for real issues address them separately. I will close this now.