Safe maths or not safe maths

Smart Contracts
1

[PERCENTAGE SHARING VAULT]

Quick overview of exercise:
Attempting to avoid maths errors in dynamic percentage split, which splits at the lowest values.

We cannot have decimals therefore the lowest value that can be split for lets say a 5% share is 20 Wei

20 * 5 / 100 = 1

1 is a stable result and it sits at the limit. We could take a cut of 19 and loose 4 wei but as it stands in the above equation we are loosing absolutely nothing. This then quickly demonstrates that the minimal amount of wei is relative to the percentage and therefore the following must always be true:

minimum_amount_of_wei * percentage = 100

The minimum value then will always be equal to = 100 / % as demonstrated below.

    function vault_share (uint256 _value) internal {
        require(_value +  VAULT_balance_major + VAULT_balance_minor < (2 ** 256) -1, "The Vault is full Thankyou");
        uint256 minimumValue = 100 / VAULT_share_percentage; 
        if (_value > minimumValue ){ VAULT_balance_minor += (_value * VAULT_share_percentage) / 100; }
        VAULT_balance_major += _value - VAULT_balance_minor;
    }

As seen any whole numbers remaining will default to the main keepers vault.

VAULT_balance_major += _value - VAULT_balance_minor;

On the flip side there is a maximum capacity and it is a uint256 with a maximum capactiy at:

(2 ** 256) -1

In WEI

115792089237316195423570985008687907853269984665640564039457584007913129639935

in ETH

115792089237316195423570985008687907853269984665640564039457

and for a laugh in USD

$301,093,011,722,900,960,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000.00 USD

What happens if you go over the limit ? How could I test this ?

sender doesn't have enough funds to send tx. The upfront cost is: 115792089237316195423570985008687907853269984665640644039457 and the sender's account (0x5b38da6a701c568545dcfcb03fcb875f56beddc4) only has: 99999999999999339164

Despite not being able to test it I may have prevented overspill by setting a limit on deposit ?

require(_value +  VAULT_balance_major + VAULT_balance_minor < (2 ** 256) -1, "The Vault is full Thankyou");

FURTHER ON THIS.
Vault privileges' are detached from the contract owner for obvious reasons, yet it is assumed that the Keeper Major is also the owner. Such deals are best made on foot, but if the contract owners wanted to control the percentage:

    function vault_share_keeper_minor_percentage (uint256 _newValue) external onlymajorVaultKeeper {
        require(_newValue >= 0 && _newValue <= 100, "Limits are 0 to 100");
        VAULT_share_percentage = _newValue;
    }

QUESTION.

I am not 100% sure this will avoid maths problems I would appreciate if Open Zeppelin could have a quick scan over it. The full code can be seen below of got from git hub see link bottom.

// SPDX-License-Identifier: MIT
// Vault.sol v0.1 by MattMcP

pragma solidity 0.8.12;
import "./security.sol";

contract Vault is security {

    uint256 internal VAULT_balance_major;
    uint256 internal VAULT_balance_minor;
    address internal VAULT_keeper_major;
    address internal VAULT_keeper_minor;
    uint256 internal VAULT_share_percentage;

    event EventVaultWithdrawal (address indexed reciever, uint256 amount);
    event EventVaultDeposit (address indexed sender, uint256 amount);
    event EventVaultKeeperTransfer (address from, address to);
  
    constructor() {
        VAULT_share_percentage = 5;
        VAULT_keeper_major = payable(msg.sender);
        VAULT_keeper_minor = payable(msg.sender);
        emit EventVaultKeeperTransfer(address(0), VAULT_keeper_major);
        emit EventVaultKeeperTransfer(address(0), VAULT_keeper_minor);
    }

    modifier onlymajorVaultKeeper() {require(msg.sender == VAULT_keeper_major, "Caller is not VAULT_keeper_major");_;}
    modifier onlyminorVaultKeeper() {require(msg.sender == VAULT_keeper_minor, "Caller is not VAULT_keeper_minor");_;}

    function vault_balance_minor() external view returns (uint256) {return VAULT_balance_minor;}
    function vault_balance_major() external view returns (uint256) {return VAULT_balance_major;}
    function vault_show_Keeper_major() external view returns (address) {return VAULT_keeper_major;}
    function vault_show_Keeper_minor() external view returns (address) {return VAULT_keeper_minor;} 

    receive() external payable {
        emit EventVaultDeposit(msg.sender, msg.value);
    }

    function vault_deposit () external payable {
        vault_share (msg.value);
        emit EventVaultDeposit(msg.sender, msg.value);
    }

    function vault_share (uint256 _value) internal {
        require(_value +  VAULT_balance_major + VAULT_balance_minor < (2 ** 256) -1, "The Vault is full Thankyou");
        uint256 minimumValue = 100 / VAULT_share_percentage; 
        if (_value > minimumValue ){ VAULT_balance_minor += (_value * VAULT_share_percentage) / 100; }
        VAULT_balance_major += _value - VAULT_balance_minor;
    }

    function vault_maximum_capacity() external pure returns (uint256) {
        return (2 ** 256) -1;
    }

    // It would be best if the deal was made on deployment this is here for testing.
    function vault_share_keeper_minor_percentage (uint256 _newValue) external onlymajorVaultKeeper {
        require(_newValue >= 0 && _newValue <= 100, "Limits are 0 to 100");
        VAULT_share_percentage = _newValue;
    }

    function vault_withdrawmajor () external onlymajorVaultKeeper portalLock {
        require(VAULT_balance_major > 0, "The Vault has been cleared");
        (bool success, ) = VAULT_keeper_major.call{value: VAULT_balance_major}("");
        require(success, "The vault is locked");
        emit EventVaultWithdrawal(address(0), VAULT_balance_major);
        VAULT_balance_major -= VAULT_balance_major;
    }

    function vault_withdrawminor () external onlyminorVaultKeeper portalLock {
        require(VAULT_balance_minor > 0, "The Vault has been cleared");
        (bool success, ) = VAULT_keeper_minor.call{value: VAULT_balance_minor}("");
        require(success, "The vault is locked");
        emit EventVaultWithdrawal(address(0), VAULT_balance_minor);
        VAULT_balance_minor -= VAULT_balance_minor;
    }

    function vault_setmajorKeeper (address _newmajorKeeper) external onlymajorVaultKeeper portalLock {
        VAULT_keeper_major = payable(_newmajorKeeper);
        emit EventVaultKeeperTransfer(address(0), VAULT_keeper_major);
    }

    function vault_setminorKeeper (address _newminorKeeper) external onlyminorVaultKeeper portalLock{
        VAULT_keeper_minor = payable(_newminorKeeper);
        emit EventVaultKeeperTransfer(address(0), VAULT_keeper_minor);
    }
}
2

Paranoia prompts me to add the likes of such

    // Have a look at the total balance of this contract
    function vault_check_net () external view returns (uint256){
        return address(this).balance;
    }
    
    // Just make sure the total amount in this contract equals the sum of both user balances.
    function vault_check_sum () internal view returns (bool){
        uint256 userBalancesSummed = VAULT_balance_major + VAULT_balance_minor;
        if (address(this).balance == userBalancesSummed){
            return true;
        }else{
            return false;
        }  
    }
    // If the contract balance does not equate to the sum of both user balances
    // Something must be wrong, this function protecting the minor users balance
    // Will allow the owner to take everything remaining from the contract. 
    function vault_emergency_withdrawal () external onlymajorVaultKeeper {
        require(vault_check_sum() == false, "There is no emergency");
        require(address(this).balance > 0, "The Vault has been cleared");
        uint256 protectMinorKeepersShare = address(this).balance - VAULT_balance_minor;
        (bool success, ) = VAULT_keeper_major.call{value: protectMinorKeepersShare}("");
        require(success, "The vault is locked");
    }

Though do I really need to add those.

3

If anyone had of looked they would have noticed a major flaw in my code.

VAULT_balance_major += _value - VAULT_balance_minor;

I believe this is why I got tokens stuck in a contract, because. (Its almost funny) if the _value deposited is less than the VAULT_balance_minor's balance then (It will either underflow) or possibly result in a balance being removed from the VAULT_balance_major. This balance will of course exist in address(this).balance but not in VAULT_balance_major. Since I did not place a function to clear address(this).balance, there it remains.

I think what I need to do is

VAULT_balance_major += address(this).balance - VAULT_balance_minor;

This should work on its own because address(this).balance is always going to >= VAULT_balance_minor...

I also edited the first line on the withdraw function to make sure all contract balance is handled.

    function vault_withdrawmajor () external onlymajorVaultKeeper portalLock {
        VAULT_balance_major = (address(this).balance - VAULT_balance_minor);
        require(VAULT_balance_major > 0, "The Vault has been cleared");