Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Issue with Decimal Offset Calculation Leading to Weak Donation Protection

oxelmiguel

Summary

In the current implementation, the calculation of decimalOffset is dynamic and based on the difference between SYSTEM_DECIMALS (18) and the decimals of the indexToken:

Source Code Reference

uint8 decimalOffset = Constants.SYSTEM_DECIMALS - IERC20Metadata(vault.indexToken).decimals();

This offset is used in the formula:

IERC4626(vault.indexToken).totalSupply() + 10 ** decimalOffset

The purpose of adding 10 ** decimalOffset is to introduce a virtual share adjustment to mitigate donation attacks.

OpenZeppelin ERC-4626 Documentation

Issues with Dynamic Decimal Offset

1. Unpredictability

  • The value of decimalOffset depends on indexToken.decimals(), leading to inconsistent behavior across different tokens.

  • If indexToken.decimals() is 18, decimalOffset is 0, effectively bypassing the intended protection.

  • If indexToken.decimals() is 6, decimalOffset is 12, which may be overly restrictive.

2. Dependency on IERC20Metadata(vault.indexToken).decimals()

  • The decimals() function returns the sum of the asset token’s decimals and decimalsOffset:

function decimals() public view virtual override(IERC20Metadata, ERC20Upgradeable) returns (uint8) {
ERC4626Storage storage $ = _getERC4626Storage();
return $._underlyingDecimals + _decimalsOffset();
}

  • decimalsOffset is set by the deployer of the ZLP vault.

    https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/zlp/ZlpVault.sol#L74

    zlpVaultStorage.decimalsOffset = decimalsOffset;

    • If an 18-decimal asset is used, decimalsOffset must be set to zero to ensure that indexToken.decimals() remains 18.

    • This means that the offset used to mitigate donation attacks will also be zero, nullifying its intended protection.

3. Weak Protection in Some Cases

  • A decimalOffset of 0 results in an added value of 1, offering no real defense.

  • A decimalOffset of 1 or 2 introduces a minor buffer but still allows donation attacks.

  • The inconsistency makes the protocol vulnerable in some cases and overly strict in others.

Impact

The protocol includes a check that reverts deposits if they result in zero shares, preventing donation attacks. However, this allows attackers to create a denial-of-service (DoS) scenario for users with small balances by increasing the minimum asset deposit required to receive at least one share. This issue arises because, for tokens with 18 decimals, no effective offset is applied, making donations inexpensive.

Proof of concept

In the createZlpVaults function used for testing, the decimalsOffset is determined by Constants.SYSTEM_DECIMALS - vaultsConfig[i].decimals:

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/script/vaults/Vaults.sol#L109

function createZlpVaults(address marketMakingEngine, address owner, uint256[2] memory vaultsIdsRange) public {
// ****
// deploy zlp vault as an upgradeable proxy
address zlpVaultImpl = address(new ZlpVault());
bytes memory zlpVaultInitData = abi.encodeWithSelector(
ZlpVault.initialize.selector,
marketMakingEngine,
Constants.SYSTEM_DECIMALS - vaultsConfig[i].decimals,
owner,
IERC20(vaultAsset),
vaultsConfig[i].vaultId
);
// ****
}

For an asset with 18 decimals:

  • The decimalsOffset in the vault is calculated as Constants.SYSTEM_DECIMALS - 18 = 0.

  • The indexToken decimals become 18 + 0 = 18.

  • The decimalOffset used for donation protection is 18 - 18 = 0.

This means that for assets with 18 decimals, no effective offset is applied, making donation attacks inexpensive and reducing the protocol’s protection against such exploits.

Proposed Solution: Fixed Decimal Offset

To ensure consistent and effective mitigation of donation attacks, we propose replacing the dynamic decimalOffset with a fixed value.

Implementation of Fixed Offset

Instead of computing decimalOffset dynamically, define it as a constant:

uint8 constant FIXED_DECIMAL_OFFSET = 6; // Chosen based on attack resistance and usability trade-offs

Note: An offset of 3 forces an attacker to make a donation 1,000 times as large.

Then modify the calculation as follows:

uint256 previewAssetsOut = sharesIn.mulDiv(
totalAssetsMinusVaultDebt,
IERC4626(vault.indexToken).totalSupply() + 10 ** FIXED_DECIMAL_OFFSET,
MathOpenZeppelin.Rounding.Floor
);
Updates

Lead Judging Commences

inallhonesty Lead Judge
6 months ago
inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

oxelmiguel Submitter
6 months ago
oxelmiguel Submitter
6 months ago
inallhonesty Lead Judge
6 months ago
inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect decimal offset calculation in VaultRouterBranch reduces donation attack protection to 1 for all tokens

Support

FAQs

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