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

Bad source of randomness #427

Open
code423n4 opened this issue Oct 25, 2022 · 3 comments
Open

Bad source of randomness #427

code423n4 opened this issue Oct 25, 2022 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates responded The Holograph team has reviewed and responded selected for report This submission will be included/highlighted in the audit report 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/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L491-L511

Vulnerability details

Impact

Using block.number and block.timestamp as a source of randomness is commonly advised against, as the outcome can be manipulated by calling contracts. In this case a compromised layer-zero-endpoint would be able to retry the selection of the primary operator until the result is favorable to the malicious actor.

Proof of Concept

An attack path for rerolling the result of bad randomness might look roughly like this:

function attack(uint256 currentNonce, uint256 wantedPodIndex, uint256 numPods, uint256 wantedOperatorIndex, uint256 numOperators,  bytes calldata bridgeInRequestPayload) external{

    bytes32 jobHash = keccak256(bridgeInRequestPayload);

    //same calculation as in HolographOperator.crossChainMessage
    uint256 random = uint256(keccak256(abi.encodePacked(jobHash, currentNonce, block.number, block.timestamp)));

    require(wantedPodIndex == random % numPods)
    require(wantedOperatorIndex == random % numOperators);

    operator.crossChainMessage(bridgeInRequestPayload);
}

The attack basically consists of repeatedly calling the attack function with data that is known and output that is wished for until the results match and only then continuing to calling the operator.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a decentralized oracle for the generation of random numbers, such as Chainlinks VRF.

It should be noted, that in this case there is a prerequirement of the layer-zero endpoint being compromised, which confines the risk quite a bit, so using a normally unrecommended source of randomness could be acceptable here, considering the tradeoffs of integrating a decentralized oracle.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 25, 2022
code423n4 added a commit that referenced this issue Oct 25, 2022
@gzeoneth
Copy link
Member

gzeoneth commented Oct 30, 2022

Duplicate of #27

@gzeoneth gzeoneth marked this as a duplicate of #169 Oct 30, 2022
@gzeoneth gzeoneth added the duplicate This issue or pull request already exists label Oct 30, 2022
@gzeoneth gzeoneth marked this as a duplicate of #27 Oct 30, 2022
@ACC01ADE ACC01ADE added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") responded The Holograph team has reviewed and responded labels Nov 9, 2022
@ACC01ADE
Copy link

ACC01ADE commented Nov 9, 2022

Very valid issue

@gzeoneth gzeoneth reopened this Nov 19, 2022
@gzeoneth gzeoneth added primary issue Highest quality submission among a set of duplicates and removed duplicate This issue or pull request already exists labels Nov 19, 2022
@gzeoneth
Copy link
Member

While sponsor noted this is a design choice to use pseudorandomness, as pointed out by the warden a compromised layer-zero-endpoint can exploit this for profit, judging this as Med risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working primary issue Highest quality submission among a set of duplicates responded The Holograph team has reviewed and responded selected for report This submission will be included/highlighted in the audit report 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