poanetwork / poa-popa

DApp for proof of physical address (PoPA) attestation for validators of POA Network
https://popa.poa.network
GNU General Public License v3.0
24 stars 18 forks source link

(Refactor) Ensure user has at least one address #145

Closed phahulin closed 6 years ago

phahulin commented 6 years ago

Before filing an issue, please provide the following information:

Make explicit check that user has at least one address registered. If not, return empty string https://github.com/poanetwork/poa-popa/blob/4a3cd0f2ab913383bbcf85ad51828acd66461ac2/blockchain/contracts/ProofOfPhysicalAddress.sol#L185-L190

fvictorio commented 6 years ago

Doing this would mean removing the checkUserExists modifier, since users are removed when their only address is removed. Are you OK with that?

phahulin commented 6 years ago

Let's keep both checkUserExists and add the new check. Auditor's concern is "for the future development" when it might be possible to have user with 0 addresses or popa contract will be separated into multiple contracts

fvictorio commented 6 years ago

Sorry, I don't follow.

The checkUserExists modifier throws an error if the user doesn't exist. If we want to return an empty string, we need to remove the modifier.

We can do something like this:

    function userLastSubmittedName(address wallet)
    public constant returns (string)
    {
        if (userExists(wallet)) {
            return users[wallet].physicalAddresses[users[wallet].physicalAddresses.length-1].name;
        }

        return "";
    }
phahulin commented 6 years ago

Oh, yes, you're right, thanks. Let me check with auditors

phahulin commented 6 years ago

@fvictorio so the concern is the following: in the current implementation users always have addresses, but it might happen in the future that a user can exist but has no addresses. So suggestion is to keep the modifier and add explicit check that physicalAddresses.length>0 before getting name. If physicalAddresses.length==0 - return empty string.