BscBridgeOptimized Manual Review Findings

BBO-01M: Insecure Elliptic Curve Recovery Mechanism

Description:

The ecrecover function is a low-level cryptographic function that should be utilized after appropriate sanitizations have been enforced on its arguments, namely on the s and v values. This is due to the inherent trait of the curve to be symmetrical on the x-axis and thus permitting signatures to be replayed with the same x value (r) but a different y value (s).

Example:

Copy
193function recoverSigner(bytes32 message, bytes memory sig)
194    internal
195    pure
196    returns (address)
197{
198    (uint8 v, bytes32 r, bytes32 s) = splitSignature(sig);
199
200    return ecrecover(message, v, r, s);
201}

Recommendation:

We advise them to be sanitized by ensuring that v is equal to either 27 or 28 (v ∈ {27, 28}) and to ensure that s is existent in the lower half order of the elliptic curve (0 < s < secp256k1n ÷ 2 + 1) by ensuring it is less than 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1. A reference implementation of those checks can be observed in the ECDSA library of OpenZeppelin and the rationale behind those restrictions exists within Appendix F of the Yellow Paper.

Alleviation:

The signature verification checks are now properly enforced in the codebase along with corresponding error declarations.

BBO-02M: Potential Invalidation of Arbitrary Transactions

Description:

The adjustment of the threshold can be performed arbitrarily by the owner and will cause all pending transactions that exactly match it to fail if the threshold is increased.

Example:

Copy
81function setThreshold(uint256 _newThreshold) external onlyOwner {
82    if (!(_newThreshold >= 2 ))
83    revert InvalidThreshold();
84    threshold = _newThreshold;
85}

Recommendation:

We advise this trait of the system to be re-evaluated and the threshold adjustment to be guarded by additional security checks, such as ensuring that it is at most equal to a percentage of the total signers of the system which should be tracked in a dedicated variable.

Alleviation:

The DAFI Protocol team evaluated this exhibit but opted not to apply a remediation for it in the current iteration of the codebase.