From e53f88e991c9979286ad61f59c2683b8f3a1bf7b Mon Sep 17 00:00:00 2001 From: Jun Kim <64379343+junkim012@users.noreply.github.com> Date: Mon, 6 May 2024 21:38:58 -0400 Subject: [PATCH] fix: querying unaccrued interest when paused, caching IDLE balance in reallocate, accrue fee when feePercentage is updated --- src/interfaces/IIonPool.sol | 1 + src/token/RewardToken.sol | 10 ++- src/vault/Vault.sol | 20 ++++- test/unit/concrete/vault/Vault.t.sol | 94 +++++++++++++++++++++++ test/unit/fuzz/vault/Vault.t.sol | 111 +++++++++++++++++++++++++++ 5 files changed, 232 insertions(+), 4 deletions(-) diff --git a/src/interfaces/IIonPool.sol b/src/interfaces/IIonPool.sol index b28d071f..b7b9e587 100644 --- a/src/interfaces/IIonPool.sol +++ b/src/interfaces/IIonPool.sol @@ -234,4 +234,5 @@ interface IIonPool { function getTotalUnderlyingClaims() external view returns (uint256); function getUnderlyingClaimOf(address user) external view returns (uint256); function extsload(bytes32 slot) external view returns (bytes32); + function balanceOfUnaccrued(address user) external view returns (uint256); } diff --git a/src/token/RewardToken.sol b/src/token/RewardToken.sol index d50aae14..39ad6cbe 100644 --- a/src/token/RewardToken.sol +++ b/src/token/RewardToken.sol @@ -450,7 +450,7 @@ abstract contract RewardToken is } /** - * @dev Current token balance + * @dev Current claim of the underlying token inclusive of interest to be accrued. * @param user to get balance of */ function balanceOf(address user) public view returns (uint256) { @@ -461,6 +461,14 @@ abstract contract RewardToken is return $._normalizedBalances[user].rayMulDown($.supplyFactor + totalSupplyFactorIncrease); } + /** + * @dev Current claim of the underlying token without accounting for interest to be accrued. + */ + function getBalanceOfUnaccrued(address user) public view returns (uint256) { + RewardTokenStorage storage $ = _getRewardTokenStorage(); + return $._normalizedBalances[user].rayMulDown($.supplyFactor); + } + /** * @dev Accounting is done in normalized balances * @param user to get normalized balance of diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol index 33d19436..a63dccc7 100644 --- a/src/vault/Vault.sol +++ b/src/vault/Vault.sol @@ -16,7 +16,6 @@ import { Multicall } from "@openzeppelin/contracts/utils/Multicall.sol"; import { ReentrancyGuard } from "openzeppelin-contracts/contracts/utils/ReentrancyGuard.sol"; import { AccessControlDefaultAdminRules } from "@openzeppelin/contracts/access/extensions/AccessControlDefaultAdminRules.sol"; - /** * @title Ion Lending Vault * @author Molecular Labs @@ -28,6 +27,7 @@ import { AccessControlDefaultAdminRules } from * * @custom:security-contact security@molecularlabs.io */ + contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, ReentrancyGuard { using EnumerableSet for EnumerableSet.AddressSet; using Math for uint256; @@ -115,6 +115,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy */ function updateFeePercentage(uint256 _feePercentage) external onlyRole(OWNER_ROLE) { if (_feePercentage > RAY) revert InvalidFeePercentage(); + _accrueFee(); feePercentage = _feePercentage; } @@ -343,6 +344,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy // to the user from the previous function scope. if (pool != IDLE) { pool.withdraw(address(this), transferAmt); + } else { + currentIdleDeposits -= transferAmt; } totalWithdrawn += transferAmt; @@ -372,6 +375,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy // contract. if (pool != IDLE) { pool.supply(address(this), transferAmt, new bytes32[](0)); + } else { + currentIdleDeposits += transferAmt; } totalSupplied += transferAmt; @@ -655,7 +660,16 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy for (uint256 i; i != _supportedMarketsLength;) { IIonPool pool = IIonPool(supportedMarkets.at(i)); - uint256 assetsInPool = pool == IDLE ? BASE_ASSET.balanceOf(address(this)) : pool.balanceOf(address(this)); + uint256 assetsInPool; + if (pool == IDLE) { + assetsInPool = BASE_ASSET.balanceOf(address(this)); + } else { + if (pool.paused()) { + assetsInPool = pool.balanceOfUnaccrued(address(this)); + } else { + assetsInPool = pool.balanceOf(address(this)); + } + } assets += assetsInPool; @@ -762,7 +776,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy (feeShares, newTotalAssets) = _accruedFeeShares(); if (feeShares != 0) _mint(feeRecipient, feeShares); - lastTotalAssets = newTotalAssets; // This update happens outside of this function in Metamorpho. + lastTotalAssets = newTotalAssets; emit FeeAccrued(feeShares, newTotalAssets); } diff --git a/test/unit/concrete/vault/Vault.t.sol b/test/unit/concrete/vault/Vault.t.sol index bb73be56..e36921ed 100644 --- a/test/unit/concrete/vault/Vault.t.sol +++ b/test/unit/concrete/vault/Vault.t.sol @@ -1495,6 +1495,100 @@ contract VaultERC4626ExternalViews is VaultSharedSetup { function test_PreviewRedeem() public { } } +contract VaultInflationAttack is VaultSharedSetup { + function setUp() public override { + super.setUp(); + } + + /** + * Starting Attacker Balance: 11e18 + 10 + * Attacker Mint: 10 shares + * Attacker Donation: 11e18 + * Alice Deposit: 1e18 + * Alice Shares Minted: + * + * How much did the attacker lose during the donation? + * Attacker Donated 11e18, + */ + function test_InflationAttackNotProfitable() public { + IIonPool[] memory market = new IIonPool[](1); + market[0] = IDLE; + + uint256[] memory allocationCaps = new uint256[](1); + allocationCaps[0] = type(uint256).max; + + IIonPool[] memory queue = new IIonPool[](4); + queue[0] = IDLE; + queue[1] = weEthIonPool; + queue[2] = rsEthIonPool; + queue[3] = rswEthIonPool; + + vm.prank(OWNER); + vault.addSupportedMarkets(market, allocationCaps, queue, queue); + + uint256 donationAmt = 11e18; + uint256 mintAmt = 10; + + // fund attacker + setERC20Balance(address(BASE_ASSET), address(this), donationAmt + mintAmt); + + uint256 initialAssetBalance = BASE_ASSET.balanceOf(address(this)); + console2.log("attacker balance before : "); + console2.log(initialAssetBalance); + + vault.mint(mintAmt, address(this)); + uint256 attackerClaimAfterMint = vault.previewRedeem(vault.balanceOf(address(this))); + + console2.log("attackerClaimAfterMint: "); + console2.log(attackerClaimAfterMint); + + console2.log("donationAmt: "); + console2.log(donationAmt); + + // donate to inflate exchange rate by increasing `totalAssets` + IERC20(address(BASE_ASSET)).transfer(address(vault), donationAmt); + + // how much of this donation was captured by the virtual shares on the vault? + uint256 attackerClaimAfterDonation = vault.previewRedeem(vault.balanceOf(address(this))); + + console2.log("attackerClaimAfterDonation: "); + console2.log(attackerClaimAfterDonation); + + uint256 lossFromDonation = attackerClaimAfterMint + donationAmt - attackerClaimAfterDonation; + + console2.log("loss from donation: "); + console2.log(lossFromDonation); + + address alice = address(0xabcd); + setERC20Balance(address(BASE_ASSET), alice, 10e18 + 10); + + vm.startPrank(alice); + IERC20(address(BASE_ASSET)).approve(address(vault), 1e18); + vault.deposit(1e18, alice); + vm.stopPrank(); + + // Alice gained zero shares due to exchange rate inflation + uint256 aliceShares = vault.balanceOf(alice); + console.log("alice must lose all her shares : "); + console.log(aliceShares); + + // How much of alice's deposits were captured by the attacker's shares? + uint256 attackerClaimAfterAlice = vault.previewRedeem(vault.balanceOf(address(this))); + uint256 attackerGainFromAlice = attackerClaimAfterAlice - attackerClaimAfterDonation; + console2.log("attackerGainFromAlice: "); + console2.log(attackerGainFromAlice); + + vault.redeem(vault.balanceOf(address(this)) - 3, address(this), address(this)); + uint256 afterAssetBalance = BASE_ASSET.balanceOf(address(this)); + + console.log("attacker balance after : "); + console.log(afterAssetBalance); + + assertLe(attackerGainFromAlice, lossFromDonation, "attack must not be profitable"); + assertLe(afterAssetBalance, initialAssetBalance, "attacker must not be profitable"); + } +} + contract VaultDeposit_WithoutSupplyFactor is VaultDeposit { function setUp() public override(VaultDeposit) { super.setUp(); diff --git a/test/unit/fuzz/vault/Vault.t.sol b/test/unit/fuzz/vault/Vault.t.sol index 7f034cc1..32cc524f 100644 --- a/test/unit/fuzz/vault/Vault.t.sol +++ b/test/unit/fuzz/vault/Vault.t.sol @@ -1,11 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.21; +import { IIonPool } from "./../../../../src/interfaces/IIonPool.sol"; import { IonPoolExposed } from "../../../helpers/IonPoolSharedSetup.sol"; import { VaultSharedSetup } from "../../../helpers/VaultSharedSetup.sol"; import { Math } from "openzeppelin-contracts/contracts/utils/math/Math.sol"; import { WadRayMath, RAY, WAD } from "./../../../../src/libraries/math/WadRayMath.sol"; import { console2 } from "forge-std/console2.sol"; +import { IERC20 } from "openzeppelin-contracts/contracts/interfaces/IERC20.sol"; using Math for uint256; @@ -42,6 +44,17 @@ contract Vault_Fuzz is VaultSharedSetup { uint256 re = assets - ((assets * RAY - ((assets * RAY) % supplyFactor)) / RAY); assertLe(expectedClaim - resultingClaim, (supplyFactor - 2) / RAY + 1); } + + function testFuzz_FullyWithdrawableFromIonPool(uint256 assets) public { + uint256 supplyFactor = bound(assets, 1e27, 10e45); + uint256 normalizedAmt = bound(assets, 0, type(uint48).max); + + uint256 claim = normalizedAmt * supplyFactor / RAY; + uint256 sharesToBurn = claim * RAY / supplyFactor; + sharesToBurn = sharesToBurn * supplyFactor < claim * RAY ? sharesToBurn + 1 : sharesToBurn; + + assertEq(normalizedAmt, sharesToBurn); + } } contract VaultWithYieldAndFee_Fuzz is VaultSharedSetup { @@ -158,3 +171,101 @@ contract VaultWithYieldAndFee_Fuzz is VaultSharedSetup { function testFuzz_MaxDepositAndMaxMint(uint256 assets) public { } } + +contract VaultInflationAttack is VaultSharedSetup { + address immutable ATTACKER = newAddress("attacker"); + address immutable USER = newAddress("user"); + + function setUp() public override { + super.setUp(); + + IIonPool[] memory market = new IIonPool[](1); + market[0] = IDLE; + + uint256[] memory allocationCaps = new uint256[](1); + allocationCaps[0] = type(uint256).max; + + IIonPool[] memory queue = new IIonPool[](4); + queue[0] = IDLE; + queue[1] = weEthIonPool; + queue[2] = rsEthIonPool; + queue[3] = rswEthIonPool; + + vm.prank(OWNER); + vault.addSupportedMarkets(market, allocationCaps, queue, queue); + + vm.prank(ATTACKER); + IERC20(address(BASE_ASSET)).approve(address(vault), type(uint256).max); + vm.prank(USER); + IERC20(address(BASE_ASSET)).approve(address(vault), type(uint256).max); + } + + function testFuzz_InflationAttackNotProfitable(uint256 assets) public { + // 1. The vault has not been used. + // - no shares minted and no assets deposited. + // - but the initial conversion is dictated by virtual shares. + assertEq(vault.totalSupply(), 0, "initial total supply"); + assertEq(vault.totalAssets(), 0, "initial total assets"); + + // 2. The attacker makes a first deposit. + uint256 firstDepositAmt = bound(assets, 0, type(uint128).max); + setERC20Balance(address(BASE_ASSET), ATTACKER, firstDepositAmt); + + vm.prank(ATTACKER); + vault.mint(firstDepositAmt, ATTACKER); + + uint256 attackerClaimAfterMint = vault.previewRedeem(vault.balanceOf(ATTACKER)); + + // check that the mint amount and transfer amount was the same + assertEq(BASE_ASSET.balanceOf(ATTACKER), 0, "mint amount equals transfer amount"); + + // 3. The attacker donates. + // - In this case, transfers to vault to increase IDLE deposits. + // - Check that the attacker loses a portion of the donated funds. + uint256 donationAmt = bound(assets, 0, type(uint128).max); + setERC20Balance(address(BASE_ASSET), ATTACKER, donationAmt); + + vm.prank(ATTACKER); + IERC20(address(BASE_ASSET)).transfer(address(vault), donationAmt); + + uint256 attackerClaimAfterDonation = vault.previewRedeem(vault.balanceOf(ATTACKER)); + uint256 attackerLossFromDonation = donationAmt - (attackerClaimAfterDonation - attackerClaimAfterMint); + + uint256 totalAssetsBeforeDeposit = vault.totalAssets(); + uint256 totalSupplyBeforeDeposit = vault.totalSupply(); + + // 4. A user makes a deposit where the shares truncate to zero. + // - sharesToMint = depositAmt * (newTotalSupply + 1) / (newTotalAssets + 1) + // - The sharesToMint must be less than 1 to round down to zero + // - depositAmt * (newTotalSupply + 1) / (newTotalAssets + 1) < 1 + // - depositAmt < 1 * (newTotalAssets + 1) / (newTotalSupply + 1) + uint256 maxDepositAmt = (vault.totalAssets() + 1) / (vault.totalSupply() + 1); + uint256 userDepositAmt = bound(assets, 0, maxDepositAmt); + + vm.startPrank(USER); + setERC20Balance(address(BASE_ASSET), USER, userDepositAmt); + IERC20(address(BASE_ASSET)).approve(address(vault), userDepositAmt); + vault.deposit(userDepositAmt, USER); + vm.stopPrank(); + + assertEq(vault.balanceOf(USER), 0, "user minted shares must be zero"); + + uint256 attackerClaimAfterUser = vault.previewRedeem(vault.balanceOf(ATTACKER)); + uint256 attackerGainFromUser = attackerClaimAfterUser - attackerClaimAfterDonation; + + // loss = donationAmt / (1 + firstDepositAmt) + uint256 expectedAttackerLossFromDonation = donationAmt / (1 + firstDepositAmt); + assertLe( + attackerLossFromDonation - expectedAttackerLossFromDonation, + 1, + "attacker loss from donation as expected with rounding error" + ); + + // INVARIANT: The money gained from the user must be less than or equal to the attacker's loss from the + // donation. + // assertLe(attackerGainFromUser, attackerLossFromDonation, "attacker must not profit from user"); + assertLe(userDepositAmt, attackerLossFromDonation, "loss must be ge to user deposit"); + } + + function testFuzz_InflationAttackSmallerDegree() public { } +}