Reviewed by: 0x52 (@IAm0x52)
Fix Review Hash: 713aee7
As an independent smart contract auditor I have completed over 100 separate reviews. I primarily compete in public contests as well as conducting private reviews (like this one here). I have more than 30 1st place finishes (and counting) in public contests on Code4rena and Sherlock. I have also partnered with SpearbitDAO as a Lead Security researcher. My work has helped to secure over $1 billion in TVL across 100+ protocols.
The index-coop-smart-contracts repo and Index-protocol repo were reviewed at commit hash 1ebd3db and commit hash 1b638bf, respectively.
In-Scope Contracts:
index-coop-smart-contracts
-contract/adapters/TargetWeightWrapExtension.sol
Index-protocol
-contracts/protocol/v1/CustomOracleNAVIssuanceModule.sol
-contracts/protocol/SetValuer.sol
-contracts/protocol/PriceOracle.sol
-contracts/protocol/integration/oracles/PreciseUnitOracle.sol
-contracts/protocol/integration/oracles/ERC4626Oracle.sol
-contracts/protocol/modules/v1/RebasingComponentModule.sol
-contracts/protocol/modules/v1/WrapModuleV2.sol
-contracts/protocol/integration/warp-v2/AaveV2WrapV2Adapter.sol
-contracts/protocol/integration/warp-v2/AaveV3WrapV2Adapter.sol
-contracts/protocol/integration/warp-v2/CompoundV3WrapV2Adapter.sol
-contracts/protocol/integration/warp-v2/ERC4626WrapV2Adapter.sol
Deployment Chain(s)
- Ethereum Mainnet
| Identifier | Title | Severity | Mitigated |
|---|---|---|---|
| [H-01] | Alternating DIMv3 deposits and NAV redemptions can be used to inflate position multiplier and steal funds via rounding loss | High | ✔️ |
| [M-01] | ERC4626Oracle#read may return incorrect value for future added ERC4626 vaults | Medium | ✔️ |
| [M-02] | WrapModuleV2 fails to consider slippage when depositing into ERC4626 vaults | Medium | ✔️ |
[H-01] Alternating DIMv3 deposits and NAV redemptions can be used to inflate position multiplier and steal funds via rounding loss
CustomOracleNAVIssuanceModule.sol#L1024-L1039
function _getRedeemPositionMultiplier(
ISetToken _setToken,
uint256 _setTokenQuantity,
ActionInfo memory _redeemInfo
)
internal
view
returns (uint256, int256)
{
uint256 newTotalSupply = _redeemInfo.previousSetTokenSupply.sub(_setTokenQuantity);
int256 newPositionMultiplier = _setToken.positionMultiplier()
.mul(_redeemInfo.previousSetTokenSupply.toInt256())
.div(newTotalSupply.toInt256());
return (newTotalSupply, newPositionMultiplier);
}
When redeeming from the NAVIM the position multiplier for the set token is increased. This scales the internal balances of the other components to account for the USDC being removed to pay for the redemption.
If mint and redeems are approximately even this is never an issue but I can be if there is large imbalance. This leads to the following attack which can inflate the position multiplier:
- Mint via DIMv3
- Unwrap yb-tokens to USDC (permissionlessly)
- Redeem via NAVIM
- Repeat
With each iteration the position multiplier is pushed higher. The opposite order can be done to deflate the position multiplier.
function _convertRealToVirtualUnit(int256 _realUnit) internal view returns(int256) {
int256 virtualUnit = _realUnit.conservativePreciseDiv(positionMultiplier);
// This check ensures that the virtual unit does not return a result that has rounded down to 0
if (_realUnit > 0 && virtualUnit == 0) {
revert("Real to Virtual unit conversion invalid");
}
// This check ensures that when converting back to realUnits the unit won't be rounded down to 0
if (_realUnit > 0 && _convertVirtualToRealUnit(virtualUnit) == 0) {
revert("Virtual to Real unit conversion invalid");
}
return virtualUnit;
}
When converting from real to virtual unit during updates the contract uses the following math:
realUnit * 1e18 / positionMultiplier
When converting back:
virtualUnit * positionMultiplier / 1e18
For very high values of positionMultiplier (i.e. 1e35) we start getting significant precision loss. Lets walk through an example with a real unit of 0.25e18:
virtualUnit = 0.25e18 * 1e18 / 1e35 = 2
realUnit = 2 * 1e35 / 1e18 = 0.2e18
We can see our 0.25e18 real unit ends up truncated as 0.2e18.
With these two concepts we can now put together an attack to drain the entire vault:
- Inflate positionMultiplier to cause precision loss
- Deposit on DIMv3. This only requires 0.2 to mint but grants ~0.25 in assets
- Deflate positionMultiplier to reverse precision loss
- Withdraw more assets than deposited
- Repeat
This can be repeated as many times as necessary to drain the vault of all funds. Due to the multiplicative nature of the positionMultiplier it take only ~50 inflation loops of 20% to increase the position multiplier to 1e22 which is what is necessary for to cause inflation loss to a 6 dp token. At with withdraw fee of 0.2% the net cost of the attack is:
0.2 * 50 * 20% = 2%
Even with fees the total cost of the attack is ~2% NAV, making it highly profitable to execute assuming reasonable gas costs.
Strict positionMultiplier bounds (i.e. max 1e20) should be placed to prevent large amounts of precision loss in the underlying units.
Fixed in commit 7052f2c3. When redeeming from the NAV issuance module, the position multiplier is limited to 1e20, preventing large amount of precision loss that can be abused.
function read() external view returns (uint256) {
uint256 assetsPerShare = vault.convertToAssets(vaultFullUnit);
return assetsPerShare * vaultFullUnit / underlyingFullUnit;
}
PriceOracle.sol assumes that all prices returned by integrated sub-oracles will return the price to 18 dp. When returned price, ERC4626Oracle.sol assumes that vaultFullUnit == 1e18. The standard practice for ERC4626 vaults is for the share precision to match that of the underlying token.
Although Gauntlet Morpho USDC is an exception with an 18 dp share, this contract is a generic ERC4626 adapter and would present a large security risk if other USDC vaults such as Yearn's yvUSDC are added to the vault at a later date. In this scenario the shares would be highly undervalued, allowing users to deposit via the NAV issuance module and withdraw from the debt issuance module to drain the vault.
vaultFullUnit should not be assumed to be 1e18. Instead a separate conversion factor should be used to ensure the price is returned to 18 dp.
Fixed in commit cf32c30 as recommended.
function _createWrapDataAndInvoke(
ISetToken _setToken,
IWrapV2Adapter _wrapAdapter,
address _underlyingToken,
address _wrappedToken,
uint256 _notionalUnderlying,
bytes memory _wrapData
) internal {
(
address callTarget,
uint256 callValue,
bytes memory callByteData
) = _wrapAdapter.getWrapCallData(
_underlyingToken,
_wrappedToken,
_notionalUnderlying,
address(_setToken),
_wrapData
);
_setToken.invoke(callTarget, callValue, callByteData);
}
When depositing into a majority of ERC4626 vaults, it is possible to encounter slippage. When making deposit calls WrapModuleV2 does so directly without considering this.
Similar to M-01, Gauntlet Morpho USDC is an exception and operates in a no-loss manner which is atypical behavior for ERC4626 vaults. This would present a large security risk if other USDC vaults such as Yearn's yvUSDC are added to the vault at a later date. In this scenario wrap/unwrap calls to the ERC4626 vaults would be MEV'd and large portions of funds would be lost.
Typically a router is used to ensure slippage is accounted for. Integrating these slippage protections would require large changes to the WrapModuleV2 which is not meant to interact with "wrappers" that can lose value on deposit or withdrawal. This leads me to my two recommendations:
- Future vaults should either be screened to ensure only no-loss operation vaults
- Or these modules should be entered/exited from a newly created/modified contract that manages slippage
Fixed in commit 98c2c9c as recommended. Warning added to ERC4626WrapV2Adapter to only use no-loss vaults with wrapper. ERC4626ExchangeAdapter has been added to allow for vaults that may incur value loss due to fees, slippage or the like