oxen-io / eth-sn-contracts Goto Github PK

I'm concerned about the security of libraries/BN256G1.sol, libraries/BN256G2.sol, and libraries/Pairing.sol, considering:

1. BN256G2.sol originates from a third-party open-source implementation that:

• has not seen industry usage
• no updates for 5 years
• containing hundreds of lines of low-level elliptic curve arithmetic operations.

2. BN256G1.sol and Pairing.sol also haven't been tested in any industry use case.

These contracts are set to bear the responsibility for assets worth tens of millions of dollars. If there are any security vulnerabilities in this code, Oxen/Session investors will be the first to suffer losses, as no one else has tested these libraries in the past.

There's a well-known lesson: Don't roll your own cryptography (unless you are an expert). I wonder how many developers on the Oxen team fully understand every single line of these low-level mathematical operations and their consequences.

I was once shocked to realize that some core developers at Oxen have made rookie mistakes by confusing point multiplication inverse and scalar multiplication inverse when devising homemade cryptography. I hate to say this, but I couldn't convince myself a team that made such an entry-level mistake could grasp every single line of these cryptographic arithmetic implementation. Perhaps you used to understand these mathematical concepts during school, but if you're not working with them daily, there are countless opportunities for mistakes.

Can you please clarify the security of these libraries? What efforts have been made to ensure their security? How confident are you in these libraries? What level of security do you aim to achieve before deploying to production with real-world assets? How many people on the team have successfully implemented an algebraic attack against an elliptic curve cryptography implementation, even just as a school assignment? If you don't know how to attack another's library, how can you know how to defend your own? How much do you know about the state of the art in attacking and defending these cryptographic algorithms?

While third-party security auditing is valuable, it must be conducted by the right experts. Smart contract security specialists are not necessarily experts in cryptographic mathematics, and you need individuals skilled in both smart contract security and cryptographic math security.

Please forgive me for being direct. I don't mean to be aggressive or offensive; I'm just very concerned.

I would like to suggest updating the README.me JS section and replacing the first yarn command with yarn install --frozen-lockfile.

The --frozen-lockfile flag ensures that the yarn.lock is not changed when installing dependencies. Otherwise the default behaviour is to bump up any dependencies within the scope of the package.json at time of install (therefore changing the yarn.lock). When you add a new dependency or update one using yarn add or yarn remove this will update the yarn.lock as normal and only then should the changes be committed to the repo.

This prevents accidental modifications to the lock file and ensuring your build are 100% reproducible.

See:

• https://classic.yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-frozen-lockfile
• yarnpkg/yarn#4147

The C++ component of eth-sn-contracts is still a work in progress, the complete design is not fully clear.

Based on the test suite, there appears to be a lack of consideration for the risk of replay attacks. This is observed in the proofOfPossession(uint32_t chainID, const std::string& contractAddress) function in the ServiceNode class, which only utilizes chainID and contractAddress as its parameters.

Is this a valid concern, or am I being overly concerned? If a sophisticated attacker, capable of modifying Oxen C++ code, stakes a service node and then unstakes later, could they reuse an 'expired' proofOfPossession? In the absence of unique, time-bound elements such as nonces or block Id in the proof, would there be a risk of replay attacks?

    uint64 public nextServiceNodeID = 1;

    /// @notice Liquidates a BLS public key using a signature. This function can be called by anyone if the network wishes for the node to be removed (ie from a dereg) without relying on the user to remove themselves
    /// @param serviceNodeID The ID of the service node to be liquidated.
    /// @param sigs0 First part of the signature.
    /// @param sigs1 Second part of the signature.
    /// @param sigs2 Third part of the signature.
    /// @param sigs3 Fourth part of the signature.
    /// @param ids An array of service node IDs.
    function liquidateBLSPublicKeyWithSignature(uint64 serviceNodeID, uint256 sigs0, uint256 sigs1, uint256 sigs2, uint256 sigs3, uint64[] memory ids) external {

Is it safe to use consecutively uint64 for ServiceNodeID rather than UUID/hash?

Are there any risks of potential race conditions and ID collisions?

Consider a scenario where the network agrees to liquidate node_1 and node_100, and two calls to liquidateBLSPublicKeyWithSignature() are submitted almost simultaneously. Is it possible for these two calls to be mined in the same EVM block, where the first call successfully deletes node_1 but also subtly updates the rest of the indices? As a result, the actual node_100 deleted by the second call might be the previously non-guilty node_101, rather than the guilty one.

In another scenario, suppose the network agrees to liquidate node_1. Could an attacker modify the non-signer set, construct a parallel liquidateBLSPublicKeyWithSignature() call with a replay attack but slightly different parameters, attempting to delete node_1 again? However, since the guilty node_1 has already been deleted, could a new non-guilty node_1, previously node_2, get deleted?

Could you please clarify the meaning of the following variables? They are not obvious from the code.

    /// @param _foundationPool The foundation pool for the token
    /// @param _liquidatorRewardRatio The reward ratio for liquidators
    /// @param _poolShareOfLiquidationRatio The pool share ratio during liquidation
    /// @param _recipientRatio The recipient ratio for rewards

    /// @return r the sum of two points of G1
    function add(G1Point memory p1, G1Point memory p2) internal returns (G1Point memory r) {
        uint[4] memory input;
        input[0] = p1.X;
        input[1] = p1.Y;
        input[2] = p2.X;
        input[3] = p2.Y;
        bool success;
        assembly {
            success := call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60)
        }
        require(success, "Call to precompiled contract for add failed");
    }

input is unit[4], which is 32*4 = 128 bytes, which is 0x80 in hex
r is a G1Point which is 32*2 = 64 bytes, which is 0x40 in hex
I don't understand why we have 0xc0 and 0x60 in call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60), is there anything I missed?

    /// @return r the product of a point on G1 and a scalar, i.e.
    /// p == p.mul(1) and p.add(p) == p.mul(2) for all points p.
    function mul(G1Point memory p, uint s) internal returns (G1Point memory r) {
        uint[3] memory input;
        input[0] = p.X;
        input[1] = p.Y;
        input[2] = s;
        bool success;
        assembly {
            success := call(sub(gas(), 2000), 7, 0, input, 0x80, r, 0x60)
        }
        require(success, "Call to precompiled contract for mul failed");
    }

input is unit[3] which is 32*3 = 96 bytes or 0x60 in hex
r is G1Point which is 32*2 = 64 bytes or 0x40 in hex
I don't understand the line call(sub(gas(), 2000), 7, 0, input, 0x80, r, 0x60) either, can anyone explain it for me?

Source: https://github.com/oxen-io/eth-sn-contracts/blob/e3f6bdc4eef75bce44b44e076cae0e6163b046e4/contracts/libraries/BN256G1.sol

Thank you.

when a contributor contributes more than 1 time then numberOfContributors misleadingly gets incremented. There's a mismatch in the intent of this variable. It also modifies the minimum contribution quite differently than how I'd expect. It disallows topping up your contribution, you can only top up to the next minimum contribution.

I think the desired outcome here is that we coalesce contributions from the same wallet rather than force them to contribute the minimum every time.

The blog post at https://oxen.io/blog/session-token-rewards-contract states:

"We’re utilising BLS signatures, allowing us to take multiple signatures and shrink them down into an aggregated signature which can be validated by our smart contract. This way we can make sure a majority (for example, 95%) of the network agrees on the amount before it can be claimed."

However, there seems to be a discrepancy between this blog post statement and the actual smart contract function described below. The function appears to define the 'majority' differently, so clarification on this matter would be appreciated.

    /// @notice Updates the internal threshold for how many non signers an aggregate signature can contain before being invalid
    function updateBLSThreshold() internal {
        if (totalNodes > 900) {
            blsNonSignerThreshold = 300;
        } else {
            blsNonSignerThreshold = totalNodes / 3;
        }
    }

One reason I sought clarification is that defining 'majority' as 95% like the blog post suggests appears too risky. This threshold enables a potential attack on the network by a 'whale' like L6qq who controls 5.41% of the network and could disrupt every signature aggregation by being non-cooperative (or by suffering a server crash if all their nodes are on the same server). A threshold of 66.6% seems more resilient to such whale attacks, or even to potential OPTF attacks in the event that OPTF's servers are compromised by a third party.

In other words, I guess the code is fine, but the blog post might require some edits.