[M-06] Missing Input Validation in setPoolInfo Function

[softstackio]

Likelihood: High

Description:
The setPoolInfo function in the StakingHbbft contract lacks crucial input validation for its parameters. This function allows users to set their pool's public key, IP address, and port number. However, it does not perform any checks on the validity or format of these inputs. The specific issues are:

1. PublicKey Validation: There is no check on the length or format of the publicKey parameter. This could allow users to input invalid or malformed public keys, potentially causing issues in other parts of the system that rely on this information.
2. IP Address Validation: The ip parameter, which is expected to be an IPv4 address, is not validated. This could allow users to input invalid IP addresses or even malicious data.
3. Port Number Validation: While the _port parameter is limited to 2 bytes by its type, there's no check to ensure it's within a valid range for port numbers (0-65535).

These missing validations could lead to several problems:
● Storage of invalid or nonsensical data
● Potential vulnerabilities if other parts of the system assume this data is valid
● Difficulty in using or interpreting the stored data correctly
● Possible DOS vectors if invalid data causes issues in dependent functions

Proof of Concept:

describe('setPoolInfo input validation', async () => {
    let stakingHbbft;
    let validator;
         beforeEach(async () => {
            const { stakingHbbft: _stakingHbbft } = await
helpers.loadFixture(deployContractsFixture);
            stakingHbbft = _stakingHbbft;
            validator = await ethers.getSigner(initialValidators[1]);
        });
        it('should accept any public key length', async () => {
            const shortPublicKey = ethers.zeroPadBytes("0xdead", 4);
            const longPublicKey = ethers.zeroPadBytes("0xbeef", 128);
            const ipAddress = ethers.zeroPadBytes("0xfe", 16);
            const port = '0x6987';
await
expect(stakingHbbft.connect(validator).setPoolInfo(shortPublicKey,
ipAddress, port))
                .to.not.be.reverted;
await
expect(stakingHbbft.connect(validator).setPoolInfo(longPublicKey,
ipAddress, port))
                .to.not.be.reverted;
});
        it('should accept invalid IP addresses', async () => {
            const publicKey = ethers.zeroPadBytes("0xdeadbeef", 64);
            const invalidIpAddress = ethers.zeroPadBytes("0xffffffff",
16); // 255.255.255.255
            const port = '0x6987';
await
expect(stakingHbbft.connect(validator).setPoolInfo(publicKey,
invalidIpAddress, port))
                .to.not.be.reverted;
});
        it('should accept any port number within 2 bytes', async () => {
            const publicKey = ethers.zeroPadBytes("0xdeadbeef", 64);
            const ipAddress = ethers.zeroPadBytes("0xfe", 16);
 
             const invalidPort = '0xffff'; // 65535, technically valid but
often reserved
await
expect(stakingHbbft.connect(validator).setPoolInfo(publicKey, ipAddress,
invalidPort))
                .to.not.be.reverted;
});
        it('should store and retrieve invalid data', async () => {
            const invalidPublicKey = ethers.zeroPadBytes("0xdead", 4);
            const invalidIpAddress = ethers.zeroPadBytes("0xffffffff",
            const invalidPort = '0xffff';
await
 16);
stakingHbbft.connect(validator).setPoolInfo(invalidPublicKey,
invalidIpAddress, invalidPort);
            const poolInfo = await
stakingHbbft.poolInfo(validator.address);
            expect(poolInfo.publicKey).to.equal(invalidPublicKey);
            expect(poolInfo.internetAddress).to.equal(invalidIpAddress);
            expect(poolInfo.port).to.equal(invalidPort);
}); });

setPoolInfo input validation
✔ should accept any public key length
✔ should accept invalid IP addresses
✔ should accept any port number within 2 bytes ✔ should store and retrieve invalid data

Recommendation:

1. Implement strict input validation for all parameters:
   ● For publicKey: Check that it's the expected length and format for your
   system's public keys.
   ● For ip: Validate that it's a proper IPv4 address format (consider using a
   library or a custom function for this).
   ● For _port: Ensure it's within the valid range for port numbers (0-65535).

2. Example implementation:

function setPoolInfo(bytes calldata _publicKey, bytes16 _ip, bytes2
_port) external {
    require(_publicKey.length == EXPECTED_PUBLIC_KEY_LENGTH,
"Invalid public key length");
    require(isValidIPv4(_ip), "Invalid IP address");
    uint16 portNumber = uint16(bytes2(_port));
    require(portNumber <= 65535, "Invalid port number");
    poolInfo[msg.sender].publicKey = _publicKey;
    poolInfo[msg.sender].internetAddress = _ip;
    poolInfo[msg.sender].port = _port;
}
function isValidIPv4(bytes16 _ip) internal pure returns (bool) {
    // Implement IPv4 validation logic here
    // This is a placeholder and needs to be properly implemented
    return true;
}

3. If possible, implement a mechanism to verify the ownership of the IP address and the control over the port, although this might be challenging to do on-chain.
4. Consider adding events to log when pool info is updated, which can help with monitoring and debugging.

[Kris-DMD]

#257 is opened for fixing the point 1

[SurfingNerd]

Got my thoughts on the pool info:

If the public key changes, it would change also the signing address for on chain transactions,
therefore it is not adviseable to even have the public key changeable.

In the current implementation, the only way for an operator that fears his validator private key is leaked, is to withdraw his stake and add a new pool, with a new private/public key pair for node operation.

Supporting to "change of validator key" might be much more complex, then just setting the field.

function isValidPublicKey(bytes32 _pubKeyX, bytes32 _pubKeyY) internal
pure returns (bool) {
   uint256 p =
0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F;
   uint256 x = uint256(_pubKeyX);
   uint256 y = uint256(_pubKeyY);
   if (x == 0 || x >= p || y == 0 || y >= p) {
       return false;
}
   // Check if the point is on the curve: y^2 = x^3 + 7 (mod p)
   uint256 lhs = mulmod(y, y, p);
   uint256 rhs = addmod(mulmod(mulmod(x, x, p), x, p), 7, p);
   return lhs == rhs;
}