Skip to content

Revert "chore: merge develop into master for Pasteur hardfork" - #683

Open
mrjorticio-hash wants to merge 1 commit into
bnb-chain:masterfrom
mrjorticio-hash:revert-669-develop
Open

Revert "chore: merge develop into master for Pasteur hardfork"#683
mrjorticio-hash wants to merge 1 commit into
bnb-chain:masterfrom
mrjorticio-hash:revert-669-develop

Conversation

@mrjorticio-hash

Copy link
Copy Markdown

Reverts #669

@hashdit-bot

hashdit-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

Pull Request Review

This revert rolls back CI hardening, hardfork configuration, governance blacklist enforcement, validator-rotation protections, and related regression tests. It also restores shell-based Git operations using PR-controlled inputs and removes validator eviction calls from malicious-vote and double-sign slashing paths.

Sensitive Content

No sensitive content detected.

Security Issues

🟠 [HIGH] PR-controlled values enable command injection in CI

File: .github/workflows/check-bsc-hardfork-bytecode.yml
The workflow interpolates hardfork and bsc values parsed from an untrusted pull-request description directly into a multiline shell script. An attacker can include shell metacharacters or command substitutions in the JSON values and execute arbitrary commands on the GitHub-hosted runner when the workflow runs.
Recommendation: Pass both outputs through a step-level env mapping and quote their expansions. Validate the hardfork name and BSC reference against strict allowlists before use.

🟠 [HIGH] Unvalidated Git reference is executed through a shell

File: scripts/check-bsc-hardfork-bytecode.ts
commitId is derived from a PR-controlled value and interpolated into execSync("... git checkout ${commitId}"). A malicious value can escape the intended command and execute arbitrary shell commands; the removal of hostname, path, and Git-reference validation also permits unexpected inputs.
Recommendation: Restore strict URL/reference validation and use execFileSync or spawnSync with separate argument arrays, such as execFileSync("git", ["checkout", commitId]), rather than invoking a shell.

🟠 [HIGH] Slashed validators may remain in the active validator set after key rotation

File: contracts/StakeHub.sol
Both maliciousVoteSlash and doubleSignSlash no longer evict the validator using its current consensus address. If evidence refers to a pre-rotation key while the active validator set contains the post-rotation key, the SlashIndicator-side eviction can miss, allowing a validator punished for malicious voting or double-signing to remain active.
Recommendation: Restore IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(valInfo.consensusAddress) in both slashing paths and retain regression coverage for consensus-key rotation races.

🟡 [MEDIUM] Access control is relaxed for signature-based voting

File: contracts/BSCGovernor.sol
The existing _castVote(uint256,address,uint8,string,bytes) function no longer rejects a blacklisted recovered account. Because notInBlackList checks the transaction sender, a blacklisted voter can submit a signed ballot through a non-blacklisted relayer. Confirm that relaxing this blacklist enforcement is intentional.
Recommendation: Restore if (blackList[account]) revert InBlackList(); and the signature-based voting regression tests.

🟡 [MEDIUM] Expired consensus addresses regain validator-admin authorization

File: contracts/StakeHub.sol
The existing _bep563MsgSender() authorization logic no longer requires consensusExpiration[msg.sender] == 0. A stale consensus address retained for slash/reward resolution can therefore resolve to the operator and authorize validator administration after key rotation. Confirm that this access-control relaxation is intentional.
Recommendation: Restore the expiration check for administrative sender resolution, while using a separate lookup path where historical consensus mappings are required for slashing or rewards.


Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant