T09 · Insecure Skill Coding Practices
Warning
- Location
- SKILL.md:156
- Finding
- Unsafe Vault Accounting Can Corrupt Share and Asset Balances## Vulnerability Details **File Location**: `SKILL.md`, lines 156-170 **Vulnerability Type**: Unsafe integer downcasting and incorrect token receipt accounting **Risk Level**: Medium ### Vulnerable Code ```solidity mintedShares = totalDeposits == 0 ? amount : (amount * totalShares) / totalDeposits; // Effects before interactions (CEI pattern) totalDeposits += uint128(amount); totalShares += uint128(mintedShares); shares[msg.sender] += mintedShares; // Interaction last asset.safeTransferFrom(msg.sender, address(this), amount); ``` ### Technical Analysis The reference vault performs explicit conversions from `uint256` to `uint128` without verifying that `amount` and `mintedShares` fit within 128 bits. Explicit narrowing conversions can truncate high-order bits rather than safely rejecting an out-of-range value. Consequently, the user's full `uint256` share balance can diverge from the truncated global accounting values. For example, during the first deposit, `mintedShares` equals `amount`. If that amount exceeds `type(uint128).max`, `shares[msg.sender]` records the full value while `totalDeposits` and `totalShares` receive narrowed values. Future share calculations then operate on corrupted global totals. The implementation also assumes that `safeTransferFrom` delivers exactly `amount` tokens. Fee-on-transfer, deflationary, or otherwise non-standard tokens can transfer fewer tokens than requested. The vault nevertheless credits the requested amount, causing its recorded assets and issued shares to exceed its actual token balance. The associated fuzz test restricts deposits to `uint128` values and uses a conventional mock ERC-20 token. It therefore does not exercise oversized deposits or fee-on-transfer behavior. ### Attack Path **Oversized-deposit path:** 1. A vault is deployed using the documented reference implementation. 2. The configured token permits a deposit whose raw token amount exceeds ` ...[truncated 1624 chars]
- Remediation
- ## Remediation Suggestions 1. Use `uint256` for `totalDeposits` and `totalShares` unless storage packing is demonstrably necessary. 2. If `uint128` storage is required, use checked conversions that revert when values exceed `type(uint128).max`, such as OpenZeppelin `SafeCast.toUint128`. 3. Measure the actual amount received using token balance differences: ```solidity uint256 balanceBefore = asset.balanceOf(address(this)); asset.safeTransferFrom(msg.sender, address(this), amount); uint256 received = asset.balanceOf(address(this)) - balanceBefore; ``` 4. Calculate and issue shares from `received`, not from the caller-supplied `amount`. 5. Alternatively, explicitly reject fee-on-transfer and rebasing tokens and document that restriction as an enforced invariant. 6. Revert when a nonzero deposit would mint zero shares. 7. Ensure all global totals, individual balances, and actual token balances remain consistent through invariant tests. 8. Add tests for: - `type(uint128).max` and values above that boundary. - Fee-on-transfer and deflationary tokens. - Tokens with unusual decimal configurations. - Rounding behavior for small deposits. - The invariant that actual assets are at least recorded liabilities. - Full deposit and withdrawal sequences under fuzzing.