Storage Proofs
Curve
DeFiLayer 1Layer 2
14,723 OP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Missing Check for Proof Length and Format

sempermutans

Summary

In ScrvusdVerifierV1.sol, the _extractParametersFromProof function only checks that the proofs array has the correct overall length but fails to validate the individual proof elements. This is in contrast to ScrvusdVerifierV2.sol, which performs more comprehensive validation of its proofs. This inconsistency could lead to the extraction of incorrect parameters, potentially affecting the accuracy of price calculations in the scrvUSD oracle system.

Vulnerability Details

Severity: Medium

Files Affected:

  • contracts/scrvusd/verifiers/ScrvusdVerifierV1.sol

Functions Affected:

  • _extractParametersFromProof()

In ScrvusdVerifierV1.sol, the proof validation is limited to checking the array length:

// ScrvusdVerifierV1.sol - Lines 88-89
RLPReader.RLPItem[] memory proofs = proofRlp.toRlpItem().toList();
require(proofs.length == PROOF_CNT, "Invalid number of proofs");

The function then proceeds to extract slot values without additional validation:

// ScrvusdVerifierV1.sol - Lines 91-95
for (uint256 i = 1; i < PROOF_CNT; i++) {
Verifier.SlotValue memory slot = Verifier.extractSlotValueFromProof(
keccak256(abi.encode(PARAM_SLOTS[i])),
account.storageRoot,
proofs[i].toList()
);
// No validation of slot.exists or other proof properties
params[i - 1] = slot.value;
}

In contrast, ScrvusdVerifierV2.sol includes more comprehensive validation:

// ScrvusdVerifierV2.sol - Lines 58-59
Verifier.SlotValue memory slot = Verifier.extractSlotValueFromProof(/* ... */);
require(slot.exists);

This inconsistency in validation could allow malformed or incomplete proofs to be processed by ScrvusdVerifierV1.sol, potentially leading to incorrect parameter extraction.

The implications become more severe when examining how these parameters are used in the oracle (ScrvusdOracleV2.vy):

# From ScrvusdOracleV2.vy
@external
def update_price(
_parameters: uint256[ALL_PARAM_CNT], _ts: uint256, _block_number: uint256
) -> uint256:
# ...
self.price_params = PriceParams(
total_debt=_parameters[0],
total_idle=_parameters[1],
total_supply=_parameters[2],
full_profit_unlock_date=_parameters[3],
profit_unlocking_rate=_parameters[4],
last_profit_update=_parameters[5],
balance_of_self=_parameters[6],
)
# ...

These parameters are then used in critical price calculations:

@view
def _raw_price(ts: uint256, parameters_ts: uint256) -> uint256:
parameters: PriceParams = self._obtain_price_params(parameters_ts)
return self._total_assets(parameters) * 10**18 // self._total_supply(parameters, ts)

If any of these parameters are incorrect due to improper proof validation, it could directly affect price calculations and potentially lead to division by zero errors or incorrect prices.

Impact

The inadequate proof validation in ScrvusdVerifierV1.sol could have several implications:

  1. Silent Use of Default Values: If a storage slot doesn't exist in the scrvUSD contract, the function would still use the default value (0) for the parameter, which could lead to incorrect price updates.

  2. Incorrect Parameter Extraction: Malformed proofs might lead to extracting incorrect data or misinterpreting the proof structure, causing price calculations based on invalid inputs.

  3. Potential Oracle Manipulation: An attacker might craft special proof inputs that exploit the lack of validation to manipulate the extracted parameters, potentially affecting price calculations in the oracle.

  4. Financial Impact: Given that these parameters are used in price calculations that may affect trading decisions, incorrect values could lead to financial losses for users of the protocol.

  5. Division by Zero Risk: If the total_supply parameter (params[2]) is incorrectly extracted as 0, it could cause division by zero errors in the oracle's price calculations.

  6. Inconsistent Behavior Between Contracts: The different validation approaches between V1 and V2 create inconsistent security profiles, where V2 is more robust against certain types of invalid inputs.

Tools Used

  • Manual code review

Recommendations

  1. Add Comprehensive Proof Validation:

    • Enhance the proof validation in ScrvusdVerifierV1.sol to match or exceed the rigor of ScrvusdVerifierV2.sol:

function _extractParametersFromProof(
bytes32 stateRoot,
bytes memory proofRlp
) internal view returns (uint256[PARAM_CNT] memory) {
RLPReader.RLPItem[] memory proofs = proofRlp.toRlpItem().toList();
require(proofs.length == PROOF_CNT, "Invalid number of proofs");
​
// Extract account proof
Verifier.Account memory account = Verifier.extractAccountFromProof(
SCRVUSD_HASH,
stateRoot,
proofs[0].toList()
);
require(account.exists, "scrvUSD account does not exist");
​
// Extract slot values
uint256[PARAM_CNT] memory params;
for (uint256 i = 1; i < PROOF_CNT; i++) {
Verifier.SlotValue memory slot = Verifier.extractSlotValueFromProof(
keccak256(abi.encode(PARAM_SLOTS[i])),
account.storageRoot,
proofs[i].toList()
);
// Add explicit validation for slot existence
// For slots that must exist:
if (i != 0) { // Skip the first index which is a filler
require(slot.exists, string(abi.encodePacked("Storage slot ", i, " does not exist")));
}
params[i - 1] = slot.value;
}
​
return params;
}

  1. Add Proof Element Validation:

    • Add validation for each individual proof element to ensure they are properly formatted:

function _validateProofElement(RLPReader.RLPItem[] memory proofElement) internal pure {
require(proofElement.length > 0, "Empty proof element");
// Add additional validation as needed based on the expected proof structure
}
​
function _extractParametersFromProof(
bytes32 stateRoot,
bytes memory proofRlp
) internal view returns (uint256[PARAM_CNT] memory) {
RLPReader.RLPItem[] memory proofs = proofRlp.toRlpItem().toList();
require(proofs.length == PROOF_CNT, "Invalid number of proofs");
​
// Validate each proof element
for (uint256 i = 0; i < PROOF_CNT; i++) {
_validateProofElement(proofs[i].toList());
}
// Rest of the existing implementation
}

  1. Standardize Validation Across Contracts:

    • Implement a shared validation function that both contracts can use to ensure consistent validation:

// In a shared library or base contract
function validateProof(RLPReader.RLPItem[] memory proof, bytes32 stateRoot, bytes32 slotHash) internal view returns (bool) {
// Implement comprehensive validation logic
return true; // Replace with actual validation
}

  1. Add Parameter Reasonableness Checks:

    • Beyond validating the proof structure, add checks to ensure extracted parameters are within reasonable bounds, especially considering their role in the oracle's price calculations:

function _validateParameters(uint256[PARAM_CNT] memory params) internal pure {
// Check that parameters are within reasonable ranges
require(params[2] > 0, "Total supply cannot be zero"); // Critical for division operation in price calculation
// Add additional checks based on parameter meaning and expected ranges
// For example, check that debt and idle values are reasonable
require(params[0] < 1e40, "Total debt unreasonably large");
require(params[1] < 1e40, "Total idle unreasonably large");
// Check that timestamps are reasonable
if (params[5] > 0) { // last_profit_update
require(params[5] < block.timestamp + 1 days, "Future timestamp not allowed");
}
}
​
function _extractParametersFromProof(...) internal view returns (uint256[PARAM_CNT] memory) {
// Existing implementation with added slot existence checks
// Add parameter validation before returning
_validateParameters(params);
return params;
}

  1. Consider Parameter-Specific Validation:

    • Add validation specific to each parameter type based on their usage in the oracle:

function _validateTotalSupply(uint256 totalSupply) internal pure {
require(totalSupply > 0, "Total supply must be positive");
require(totalSupply < 1e40, "Total supply unreasonably large");
}
​
function _validateProfitTimestamps(uint256 fullProfitUnlockDate, uint256 lastProfitUpdate) internal view {
if (fullProfitUnlockDate > 0) {
require(fullProfitUnlockDate >= lastProfitUpdate, "Full profit unlock date before last update");
require(fullProfitUnlockDate < block.timestamp + 365 days, "Full profit unlock date too far in future");
}
}
​
// Use these in _validateParameters or directly in the extraction function

By implementing these recommendations, particularly the comprehensive proof validation and slot existence checks, the contract would be more robust against invalid or malicious inputs, reducing the risk of incorrect parameter extraction and improving overall security of the price oracle system.

Updates

Lead Judging Commences

0xnevi Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-slot-not-check-verifierv1-v2

- Looking at the OOS `StateProofVerifier` and `MerklePatriciaProofVerifier` contract that extracts the slot, the `exists` flag will be flagged as true as long as a non-zero length value is returned as seen [here](https://github.com/curvefi/curve-xdao/blob/3ff77bd2ccc9c88d50ee42d2a746fc7648c7ff2c/contracts/libs/StateProofVerifier.sol#L133C13-L136). From the `MerklePatriciaProofVerifier.extractProofValue`, the minimum length returned will be 1 as represenetd by `bytes(0)`. So this seems to be purely a sanity check that might not even be required. - A slot with zero values is only allowed when the proof provided by the prover correctly proofs that such values are included within the Merkle-Patricia-Tree. The values fetched from mainnet from the V3Vault stored in the merkle trie is likely checked before hand and aggregated into the MerkleTree.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.