ICP Neuron Age is 52 Years

I ran into a problem at work a while back when generating certificates. The OS was still using a 32-bit signed integer to store time. So anytime I tried to extend the certificates expiration date past a certain point in 2038 it would cause the date range circle back to the Linux Epoch. Not sure if that’s related to this issue but figured it was worth mentioning.

4 Likes

Could it be something to do with neuron merging?

Line 553

/// Given two quantities of stake with possible associated age, return the
/// combined stake and the combined age.
pub fn combine_aged_stakes(
    x_stake_e8s: u64,
    x_age_seconds: u64,
    y_stake_e8s: u64,
    y_age_seconds: u64,
) -> (u64, u64) {
    if x_stake_e8s == 0 && y_stake_e8s == 0 {
        (0, 0)
    } else {
        let total_age_seconds: u128 = (x_stake_e8s as u128 * x_age_seconds as u128
            + y_stake_e8s as u128 * y_age_seconds as u128)
            / (x_stake_e8s as u128 + y_stake_e8s as u128);

        // Note that age is adjusted in proportion to the stake, but due to the
        // discrete nature of u64 numbers, some resolution is lost due to the
        // division above. Only if x_age * x_stake is a multiple of y_stake does
        // the age remain constant after this operation. However, in the end, the
        // most that can be lost due to rounding from the actual age, is always
        // less than 1 second, so this is not a problem.
        (x_stake_e8s + y_stake_e8s, total_age_seconds as u64)
    }
}
1 Like

(deleted in case what I said was the issue and it’s exploitable)

Definitely see what it is now.

1 Like

Hi all,

Here is an update on the neurons with a large age!

Situation
As already mentioned in this thread, there are some NNS neurons that have an age which is very large. To the best of our knowledge, there are only a handful of these neurons in total.
These neurons have a slightly higher voting power bonus than the NNS design intended for. The maximum age bonus that these neurons can get is 25%. Also, the age is capped at 4 years in the code, so larger age values do not have additional effect on the voting power.

Nevertheless, we of course take this situation very seriously and treat this as a security incident.

Cause of the bug
We were able to narrow down the source of the bug to the recently changed function merge_neurons of NNS governance’s manage neuron commands. This function allows users to merge a “source neuron” into a “target neuron”, combining their stake and all their information.
In the case where the target neuron is dissolved, i.e. has zero dissolve delay, there was a bug that incorrectly updated the neurons’ age, which led some of the neurons to have these very large age values.

Stopgap proposal
To make sure that this vulnerability cannot be exploited by attackers, as a stop gap we submitted proposal [123434] to the NNS that proposed to disable merge_neurons. This proposal has been executed, and thus no additional neurons can get into this buggy state and no attackers can take advantage of this bug.

To ensure that the proposal does not reveal the bug to attackers and in accordance with the Security Patch Policy and Procedure that was adopted in proposal [48792], the source code that was used to build this release will be exposed shortly. Once the source code is revealed, the community will be able to retroactively verify the binaries that were rolled out.

Why did we choose a stopgap rather than waiting for a bug fix?

  • We think it is important to take the time for additional reviews to ensure that all paths that can lead to the bug were covered as well as for the usual security reviews and release tests.
  • Even though disabling the functionality may cause an inconvenience for some users, merging neurons is just a UX improvement that does not affect the core functionality of governance. Most importantly, users can still vote, get voting rewards, modify the neurons, and disburse stake and maturity.
  • Another advantage of this approach is that the community will be able to verify the fix before voting on the proposal, which will provide more confidence in it.

What are the next steps?
Now that the stopgap was executed by the NNS, we are working on the following tasks:

  1. Fixing the bug, making sure we do additional reviews to avoid similar bugs. As mentioned, we plan to follow the normal release process in this case and the community will be able to fully verify the code as it is merged since there is no risk of attackers exploiting the vulnerability anymore.
  2. Recovery of the affected neurons. This includes identifying the neurons that were affected by the bug and setting their age back to an expected value.

We plan to provide another update when we have a concrete proposal ready for these points!

22 Likes

Thank you @lara and the rest of the DFINITY team. This seems like a reasonable plan.

5 Likes

What do you mean thank you ? for what ? Dfinity had one job. To stop things like this from happening and guess what they failed. Nobody tested the so called “recently changed function merge_neurons of NNS governance’s manage neuron command” is what we are supposed to believe, and on top of that nobody caught it from the team months later until reported by the community? Makes you wonder what else is buggy and exploitable that hasn’t caught the eye yet.

3 Likes

Spoiler alert! There’s no perfect world. Mistakes happen what matters is how smoothly and effortlessly they get solved :white_check_mark:

7 Likes

Hello DFINITY Team, I am the owner of that neuron. The problem appears some weeks ago, I was trying to merge that neuron with a spawn neuron from maturity reward. (I was just curious about how it work) the next day when the reward was payd it wasn’t payed to this neuron but instead it appears a new neuron with cero ICP but with the maturity reward. Same day something similar happend with my OC Neuron, I disolved one neuron sent it to main account then increase another neuron staking, but next day it appears a new neuron with cero CHAT but with maturity. Now i cant do nothing with this maturity because there is no merge neuron in OC neurons. If I can help with anything else, I’m willing to.

4 Likes

Core issue seems to be with dissolving neurons

Comment states: when dissolving aging_since_timestamp_seconds is a value in the far future

let aging_timestamp_seconds_delta = (new_aging_since_timestamp_seconds as i128)
            .saturating_sub(target_neuron.aging_since_timestamp_seconds as i128);

  // --- Public interface of a neuron.

    /// Return the age of this neuron.
    ///
    /// A dissolving neuron has age zero.
    ///
    /// Technically, each neuron has an internal `aging_since`
    /// field that is set to the current time when a neuron is
    /// created in a non-dissolving state and reset when a neuron is
    /// not dissolving again after a call to `stop_dissolve`. While a
    /// neuron is dissolving, `aging_since` is a value in the far
    /// future, effectively making its age zero.
    pub fn age_seconds(&self, now_seconds: u64) -> u64 {
        now_seconds.saturating_sub(self.aging_since_timestamp_seconds)
    }

aging_since_timestamp_seconds is then relied upon to calculate the delta…

let new_aging_since_timestamp_seconds = now.saturating_sub(new_age_seconds);
        let aging_timestamp_seconds_delta = (new_aging_since_timestamp_seconds as i128)
            .saturating_sub(target_neuron.aging_since_timestamp_seconds as i128);

delta later negated to a positive value to be used as the new aging_timestamp_seconds value

(the main age_seconds calculation was a saturating subtract on an unsigned integer, so negative is not possible)

// Reset aging
            aging_timestamp_seconds: if source_stake_less_transaction_fee_e8s > 0 {
                now.saturating_sub(source_neuron.aging_since_timestamp_seconds) as i128
            } else {
                0
            },
source_neuron_mut.aging_since_timestamp_seconds =
                        saturating_add_or_subtract_u64_i128(
                            source_neuron_mut.aging_since_timestamp_seconds,
                            original_delta_aging_since_timestamp_seconds.saturating_neg(),
                *emphasized text*        );

Pretty sure the dissolving neuron’s spoofed-to-future aging_since value is the root cause. The value [max u64 number] is later used as the basis for a delta in the merge neuron function

2 Likes

I agree, its a shame. But Dfinity code is audited right ?

Btw, how about Dfinity rewarding the people that found out this issue ? Afterall a good list of daily check testing that stuff are the way they are supposed to isnt hard to do and it obviously required.

3 Likes

I guess you don’t work in software engineering…

8 Likes

On the other hand, the bug also shows need of improvement in unit and integration testing - automated unit testing of the function - like pretend merging of neurons in several states and validate output (kinda dry-run) - would reveal this…

6 Likes

Agreed, unit tests are lacking in coverage if something like this can make it through.

2 Likes

Bugs happen but it’s also important to be strict about changes to protocol.

1 Like

Just be grateful this didn’t happen to an ETH contract, otherwise the exploit would be fixed into the protocol for eternity.

With something as complex as a decentralized cloud, bugs happen. I’m just really glad that it wasn’t something way worse which impacted the network.

5 Likes

This is the followup forum thread, detailing next steps and explaining the issue.

3 Likes

Thanks @cryptodriver. I have answered to the tweet pointing to the description and solution.

6 Likes

I think there are more than 4,200 neurons with age older than genesis. Anything older than May 10 2021 is wrong, isn’t it ?