Banking System console app in C++

Question

I built a banking system in C++ (still a lot of features to do but (I decided to post it here to see what can I change so I wont make those mistakes in the future).
User header and c++ files
Header

#pragma once

#include <string>

class User
{

private:
    int id;
    std::string username;
    std::string password;
    std::string balance;

public:
    User(int id, std::string username, std::string &password, int balance);


    ~User();

    int GetId();

    std::string GetUsername();
};

CPP

#include <iostream>

#include "HashPassword.h"
#include "User.h"





    User::User(int uId, std::string uUsername, std::string& uPassword, int uBalance)
    {
        id = uId;
        username = uUsername;
        password = uPassword;
        HashPassword(password);
        std::cout << password << '\n';
        balance = uBalance;
        std::cout << balance << '\n';
    }

    
    
    User::~User()
    {
    }

    int User::GetId()
    {
        return id;
    }

    std::string User::GetUsername()
    {
        return username;
    }

all of this is pretty easy to understand (I hope) but a couple of point: first of all the HashPassword method will be posted here so scroll down a bit for the implementation, but basically I used the picosha2 header file I got from GitHub and it does some magic I don't understand and hashes a string using sha256 algorithm.
second point I got a weird feeling about how I use pointers and pass by reference here so I would like some directions on that topic.

HashPassword

#include "HashPassword.h"


void HashPassword(std::string& password)
{
    std::vector<unsigned char> hash(picosha2::k_digest_size);
    picosha2::hash256(password.begin(), password.end(), hash.begin(), hash.end());

    password = picosha2::bytes_to_hex_string(hash.begin(), hash.end());


}

this is my hashing process I took straight from the GitHub example and I have a broad idea on how it works but I don't understand it deeply.
my main concern here is the use of pointers and references.

UserRepository

#include <iostream>
#include <vector>
#include "User.h"

class UserRepository
{
private:
    std::vector<User> userRepo;


public:
    UserRepository()
    {

    }
    ~UserRepository(){}

    void AddUser(std::string& username, std::string &password, int& balance)
    {
        
        int id = userRepo.size() + 1;
        User user(id, username, password, balance);
        userRepo.push_back(user);

    }


    void PrintAllUsers()
    {
        for(User user : userRepo)
        {
            std::cout << user.GetUsername() << ", ";
        }
        std::cout << '\n';
    }

    User* FindUserByUsername(std::string username)
    {
        for (User user : userRepo)
        {
            if (user.GetUsername() == username)
            {
                User* p_user = &user;
                return p_user;
            }
        }

        return NULL;
    }
};

Here im using a vector as a repository that, and I just add users to it via AddUser and the id gets generated automatically based on how many users there are so if there are 7 users and another one joins then his id will be 8.
and this is it for now, working on adding authentication process and changing the way I go about using pointers and references.

Answer 1

Preface: Code reviews can happen on different levels. Others have already pointed out many of the issues regarding potential bugs, syntax, stylistic choices and idiomatic use of the language. This answer tries to review the code regarding the overall design and fit to the (implied) "requirements", especially as the question starts with

(still a lot of features to do but (I decided to post it here to see what can I change so I wont make those mistakes in the future).

Given you are implementing a "banking system", it is a bit unexpected to see "user" as the first thing to implement. I would expect domain terms like "customer" and "(bank) account" to be reflected in the code first. Your User class seems to model kind of both, but it also focuses on the "less important" aspects of the domain. It is currently concerned mainly with the aspects of authenticating a user via a basic username/password combination (it actually does not implement that yet, but I assume this is your intent). It does have a "balance", which would cover the "account" aspect to a very limited degree.

I would recommend to start with a proper analysis of the domain and come up with a design where each domain concept is represented separately in your code and also that the various operations and processes that need to happen in your banking system are also explicitly represented and potentially separated.

Based on my simplistic understanding of a "bank", you would have at least:

• A Customer class, which represents a single customer of your bank. That typically includes name and contact/address information as well as a customer number/ID for internal references. It likely will have additional administrative information like when they first became a customer or if they are still an active customer or not (maybe you have to delete/anonymize personal data after a certain time?). A customer does not have a password or a balance, as these are separate concepts and concerns.
• A Account class, which represents a bank account which belongs to one (or many?) customer. It would have an account number and provides the operations to compute its balance, put money into it, withdraw money or transfer money from one account to another (possibly to/from another bank?) and most importantly, list all transactions which took place on it (may need to be limited at some point?). This already indicates that it cannot just store a simple balance, but that it needs to store "transactions" which happen over time. So, we also need...
• A Transaction class. This would hold information about what amount of money was transferred between which accounts by whom and when. Typically, transactions are immutable and the list of transactions on an account is "append-only".

Based on that you would then have some kind of repositories to manage all of your customers and accounts. That is similar to what you have with the UserRepository already. As your entities have a primary key (customer number or account number), it would make sense to use a std::map to get a fast lookup of those by that key. If you want to search customers by their name or other attributes, that would be implemented in additional methods. I would strongly suggest to not have functionality like printAllUsers in there. Instead the repository only provides the access methods to get an iterator for users/accounts/customers/... and implement the user interface related functionality elsewhere (the same applies to the "entity" classes themselves too!).

At this point, you have to think about where the business constraints and rules can be implemented. For example, where do you check if a transaction should actually be carried out on an account? You may need to put those into additional classes/functions as they operate on a higher level and a single entity.

Another thing to think about how it can be modeled/represented in your system is the fact that banks usually do not exist in isolation, but (have to) allow transferring money to and from other banks.

Only then would you start to model aspects of an "online banking system", which would then have things like "users" with authentication related information. There, it would in this day and age also be more likely that the authentication part is not a simple username/password combination, but possibly some sort of MFA. In addition, you would have to think about how individual transactions are authorized by the user (in the old days, this was done with pre-generated TAN lists, nowadays it would be some kind of "OTP token" system).

Of course, as this project is likely just an excercise and/or for fun, many points can be left out and shortened to reduce the overall scope. E.g. only allowing transactions within the bank and only providing username/password authentication.

Answer 2

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.

Answer 3

User(int id, std::string username, std::string &password, int balance)

Passing the username by value will most probably create an unnecessary copy of the string. The preferable way to pass it is by const& or if used in modern C++ (17+) std::string_view.

In this function's implementation you used different names for the arguments than in the declaration. The reason is to avoid the mention of this when assigning the variables.

You can use different conventions to avoid that like naming class variables with m_variableName, and this way you can avoid strange naming in the parameters.

Another possible way in this case is to use initialization list:

User::User(int id, std::string username, std::string& password, int balance)
          : id(id), username(username), password(HashPassword(password)), balance(balance)
{
    std::cout << this->password << '\n';
    std::cout << this->balance << '\n';
}

Pay attention that some places won't like the idea of calling a function from the initialization list (like HasPassword), for reasons like debug friendly code, but I personally don't agree with that argument.

For your GetUsername function, you might want to consider returning a std::string_view or const std::string& instead, for performance issues.

For your HashPassword function, it might be better for this function to return a std::string of the hashed password, instead of modifying the original one. This function is more like a library function, and the standard usually doesn't modify the argument in these cases.

User user(id, username, password, balance);
userRepo.push_back(user);

Now here it's a classic case to use emplace_back instead of push_back, because in this case you actually use 2 constructors for User. One that you can see explicitly, and another one to copy the User instance to the push_back call / for the insertion to the vector. When using emplace_back you can use one constructor instead:

userRepo.emplace_back(id, username, password, balance);

void PrintAllUsers()
{
    for(User user : userRepo)
    {
        std::cout << user.GetUsername() << ", ";
    }
    std::cout << '\n';
}

In this function, it's better to iterate over userRepo with const User& to avoid copies of all the users. This way you will iterate using pointers instead, which is much cheaper in this case.

User* FindUserByUsername(std::string username)
{
    for (User user : userRepo)
    {
        if (user.GetUsername() == username)
        {
            User* p_user = &user;
            return p_user;
        }
    }

    return NULL;
}

In this function I want to talk about several things.

1. You can pass as argument const std::string& (or std::string_view since C++17).
2. Same issue of iterating with const& instead by value.
3. Your intention was to return a pointer from the vector, and I want to talk about the issues in your intentions before talking about the actual thing that happens here.

When returning a pointer from the vector, you expose yourself to the risk of iterator invalidation, and you actually break the encapsulation of the users that are being stored inside the class. It might be better to return a copy or const User&, depends if performance are in demand here.

However, that's not the actual case here. Because you iterate over your vector by value, you actually return a pointer to the temporary variable in the loop. Which means, that you return a pointer to an address that you don't have access outside of the loop scope. This is a bug.

4. There is no need for the line User* p_user = &user;, you can simply use return &user.
5. When returning a null, use nullptr keyword instead of the C macro NULL.

Answer 4

A banking system will inevitably require some level of concurrency, and your method of deriving the User id is not thread safe. Two (or more) threads could check the size of the userRepo at the same time, and then generate the same ID.