Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using knowledge from previous blocks can result in Backup Operator manipulation #169

Closed
code423n4 opened this issue Oct 24, 2022 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists responded The Holograph team has reviewed and responded sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1185-L1193
https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L522-L533

Vulnerability details

Using knowledge from previous blocks can result in Backup Operator manipulation

Impact

In contracts/HolographOperator.sol#crossChainMessage, after an Operator is selected (see related issue H001), the remainder job details are filled (L522-L533). This includes a call to _randomBlockHash, providing the random number used to select a particular pod, the pod size (podSize), and a sequential number (n) for each of the five Backup Operators.

The index for each Operator is calculated as (random + uint256(blockhash(block.number - n))) % podSize. This approach has three problems:

  1. Using the same random as a criterion to select both the pod as the Backup Operators
  2. Using previously pre-determined knowledge as a second criterion to select the Backup Operator
  3. Modular arithmetic properties (see Integers modulo n), where (A+B) % C == ( (A%C) + (B%C) ) % C.

In some instances, an attacker may correctly predict in which indexes a Backup Operator will be chosen. Regarding 1., every time a pod is selected, all random numbers will have a predetermined characteristic (modulus result equals X). When combining 1. with 3., we have a fixed set of possible indexes. An attacker can then monitor a particular pod size and, by easy access to the second criterion (2. previous n blockhash), can automate and influence which indexes will be selected as Operator Backups.

Impacts for this issue include:

  • Honest Operators may feel compelled to leave the protocol if there are no financial incentives (by not being selected as Backup Operator)
  • An attacker increases his odds to cover a particular or every possible backup position
  • Sophisticated attackers can monitor the network and front run to targeted attacks

Proof of Concept

For the sake of simplicity, consider the following scenario:

number_of_pods = 12
pod0_size = 10

next_randoms = 46974604320
previous_5bx = [[18615198254, 30465177259, 91796229719, 72927401278, 58415385509], ...]

At this stage, an attacker Mallory has joined pod0, with an Operator (B1) in index 2. She keeps an eye on its size, because she knows that whenever a random (% number_of_pods = 0) fall to pod0, if the pod0_size is close to 12, there's an opportunity to influence the chosen index due to the above-mentioned point 3. So, without manipulation:

random = 46974604320
selected_pod = 46974604320 % number_of_pods  //= 0, pod0 selected

backup1 = (46974604320 + 18615198254) % pod0_size //4, selected index
backup2 = (46974604320 + 30465177259) % pod0_size //9, selected index
backup3 = (46974604320 + 91796229719) % pod0_size //9, selected index
backup4 = (46974604320 + 72927401278) % pod0_size //8, selected index
backup5 = (46974604320 + 58415385509) % pod0_size //9, selected index

After checking for potential benefits of the previous BX, Mallory deploys two additional Operators, pod0_size increases to 12 and whichever random falls to pod0 won't have any effect in choosing the next backups, since random % 12 == 0:

random = 46974604320 //same is true for every random % 12 == 0
selected_pod = 46974604320 % number_of_pods  //= 0, pod0 selected

//since mallory deployed two additional operators, pod0_size = 12

backup1 = (46974604320 + 18615198254) % pod0_size //2, selected index -- her spot
backup2 = (46974604320 + 30465177259) % pod0_size //11, selected index -- new spot after deployed, this was actually not intended for this poc
backup3 = (46974604320 + 91796229719) % pod0_size //7, selected index
backup4 = (46974604320 + 72927401278) % pod0_size //10, selected index -- new spot after deployed
backup5 = (46974604320 + 58415385509) % pod0_size //5, selected index

Mallory needs to keep her place in the pod to execute the job at the right time (she'll be the first backup), which may not be as harder as it seems due to the pop mechanism (swapping with the last spot), she has good odds to keep her spot or, as a safeguard, ensure that the last position is hers in case she is selected for another job.

Although this PoC shows a fairly easy way how an attacker can take advantage of this design flaw, extreme scenarios may need some additional requirements (in a mev abundant world, if there is money to be made, it is likely to happen):

  • Monitoring the mempool
  • Front running state altering transactions
  • Targeted attacks in pods with a larger number of Operators may need more influence

Tools Used

Manual

Recommended Mitigation Steps

Has stated in M001-Biased distribution, use two random numbers for pod, Operator selection and now backup selection. Ideally, an independent source for randomness should be used, but following the assumption that the one used in L499 is safe enough, the random may have enough entropy to serve the additional requirements (use right shift to and divide into 6 blocks).

Additionally, since randomness in the blockchain is always tricky to achieve without an oracle provider, consider adding additional controls (e.g. waiting times before joining each pod) to increase the difficulty of manipulating the protocol.

And finally, in this particular case, removing the swapping mechanism (moving the last index to the chosen operator's current index) for another mechanism (e.g. shuffling) could also increase the difficulty of manipulating a particular pod.

@gzeoneth
Copy link
Member

referenced the wrong issue, ignore the above dupes

@gzeoneth gzeoneth added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Oct 30, 2022
@gzeoneth gzeoneth added the primary issue Highest quality submission among a set of duplicates label Oct 30, 2022
@gzeoneth
Copy link
Member

also see #167 #168 from the same warden with some high quality analysis

@ACC01ADE
Copy link

ACC01ADE commented Nov 9, 2022

Really good issue here. Really appreciate the #167 and #168 analysis as well.

@ACC01ADE ACC01ADE added the responded The Holograph team has reviewed and responded label Nov 9, 2022
@gzeoneth gzeoneth added duplicate This issue or pull request already exists and removed primary issue Highest quality submission among a set of duplicates labels Nov 19, 2022
@gzeoneth gzeoneth added 3 (High Risk) Assets can be stolen/lost/compromised directly and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 21, 2022
@rotcivegaf
Copy link

I reported as low in my QA report L-06, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists responded The Holograph team has reviewed and responded sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants