Critical: takeUserHistory() allows anyone to overwrite patient medical records with no access control

[Rama542]

Summary

The takeUserHistory() function in UserSide.sol is marked public with no authorization check whatsoever. This means any wallet on the network can call it at any time and silently overwrite the stored medical history of any registered patient. This is a data integrity and patient safety vulnerability at the core of the contract.

The vulnerable function

function takeUserHistory(
    uint256 _userId,
    bool _isHandicap,
    bool _isBp,
    bool _isDiabetes,
    uint256 _userExp
) public {
    PatientHistory memory pt1 = PatientHistory(
        totalUsers,
        _isHandicap,
        _isBp,
        _isDiabetes,
        _userExp
    );
    userIdtoPatientHistory[_userId] = pt1;
}

File: MedETH/foundry/src/UserSide.sol at line 122

What an attacker can do right now

Because the function accepts a _userId as input and writes directly to userIdtoPatientHistory[_userId] without checking who msg.sender is, any externally owned account can do the following:

• Pick any patient ID on the contract
• Call takeUserHistory(targetId, true, true, true, 0) and mark that patient as handicapped, hypertensive, and diabetic
• Do the same with false values and silently wipe out a patient's real medical flags
• Repeat this indefinitely with no gas cost above the base transaction fee

There is no event emitted, no ownership check, and no revert condition. The tampered data just sits there and looks exactly like legitimate data.

Why this is especially serious

This is a healthcare contract. The PatientHistory struct stores conditions like isHandicap, isBp, and isDiabetes. Downstream contracts, frontends, or billing logic that read from userIdtoPatientHistory would operate on whatever the last writer put there, not on the actual medical truth. In a real deployment this could affect insurance decisions, prescription eligibility, or billing calculations.

The irony is that the function exists for a legitimate reason. It is called internally by createUser() to initialize a blank history for a new user. The internal call is fine. The problem is that it was left public instead of being restricted or made internal.

Steps to reproduce

Deploy the contract to any testnet. Register a patient. Then from a completely different wallet that has nothing to do with that patient, call:

takeUserHistory(patientUserId, true, true, true, 999)

The call succeeds. Reading userIdtoPatientHistory[patientUserId] now returns the forged values.

Suggested fix

The simplest and most correct fix is to make the function internal since it is only ever called from within createUser(). There is no legitimate reason for an external caller to invoke it directly.

function takeUserHistory(
    uint256 _userId,
    bool _isHandicap,
    bool _isBp,
    bool _isDiabetes,
    uint256 _userExp
) internal {

If there is a future requirement to let a patient update their own history, a separate updateMyHistory() function should be added that checks userWalletAddresstoUserId[msg.sender] == _userId before writing anything. That keeps the initialization path separate from the update path and makes the authorization logic explicit.

Related

This is similar to the access control gap that was fixed in uploadMedicalReprt() in DoctorSide.sol in a recent commit. The same class of problem exists here and deserves the same treatment.

Severity

Critical. The function modifies sensitive patient health data with no access control on a public blockchain where anyone can call any public function.