Return to Revisions

Please Provide Complete Code

This sample doesn’t compile, because "hashpassword.h" is missing (and seems to rely on an external picosha library.)

Use the Correct Types

You properly are using signed fixed-point math for balances. (At least, that’s what I assume you’re doing; you never document what units the balance is being stored in.) However, you overuse int here. On some platforms, that’s only 16 bits wide (one of which is the sign bit). If a bank needs to track fractions of a cent for correct rounding, even 31 value bits will overflow for a balance in the seven figures, which could realistically happen.

I would therefore recommend int_least64_t from <cstdint>. If you really, truly want something that’s short, simple and at least 32 bits wide, though, at least use long. You might also want a declaration similar to

using Balance = std::int_least64_t;

This makes it feasible to change the type of a bank balance later.

Another place where you shouldn’t be using int is:

int id = userRepo.size() + 1;

The size will be a size_t, which can overflow an int (in as few as 32,767 accounts on some implementations). Prior to C++23, this would be undefined behavior, which compilers today interpret as permission to silently introduce security bugs. In C++23, it will yield duplicate user IDs. Granted, you might be confident that your bank will never run this software on 16-bit CPUs and never sign up a quarter of the world’s population as customers. But still, on principle, use size_t or some big-enough unsigned counter for your account IDs.

This is one of a few things that makes me suspect you learned C# first. In C#, Int is exactly 32 bits wide. In C++, int is a type that we dinosaurs rarely use, because we remember the ’90s, when it was different sizes on different machines.

Avoid Global Variables

At present, you have a single userRepo global variable. Passing a Repo& to every function in the program (the Tramp Data pattern) might not be an improvement, but you should consider it if only because it gives you the flexibility to maintain multiple smaller repos that can be searched separately, for instance if you later acquire another bank or open a second branch.

Use Consistent Naming Patterns

You’re declaring your member functions in PascalCase, but variables in camelCase. This is common in C#, but it’s much more common in C++ to use PascalCase for class names and camelCase for class members. The other common convention is snake_case for everything.

Password Security

You appear to be storing the SHA-256 hash of the password, presumably to protect your passwords from someone with a copy of the database. This is vulnerable to dictionary or rainbow-table attacks, which compute the hashes of commonly-used passwords.

Without going into the weeds of all the things people might do to improve security, one very common technique to defend against this is to add a password "salt." That is, you randomly generate a few extra bytes such as \xA4\x2B\x76\xAF, different for each user, so that if the user chooses the password asdfghjk, you actually compute the hash of something like asdfghjk\xA4\x2B\x76\xAF or the XOR with 0xA42B76AF. This means an attacker can no longer just search your database for the hashes of password, qwertyuiop, and so on. Since the salts are stored in the database along with the hashes, an attacker can still pick an account, see its salt, then try a rainbow-table attack on that one account. Another common technique is to add "pepper." That is, pick a secret value to modify all the hashes, and don’t store it in the same files as the salts or hashes. That way, an attacker can only guess at the passwords of your dumb rich clients if they also obtain your source code, or put a lot of work into reverse-engineering the executable.

I don’t get the impression that robust security is the focus of this exercise, but since you asked for a review of the security code, that’s where I’d start.