Skip to content

Latest commit

 

History

History
91 lines (74 loc) · 3.28 KB

072.md

File metadata and controls

91 lines (74 loc) · 3.28 KB
Raw

Cuddly Cinnabar Swan

High

Max withdrawal will cost protocol lost of 5% of the total fee to be collected

Summary

Max withdrawal will cost protocol lost of 5% of the total fee to be collected

The quary of what maxAssest of the user should be uses previewRedeem(totalSharesOfUser), which deduct fees

    function previewRedeem(uint256 shares) public view override returns (uint256 assets) {
        UsualXStorageV0 storage $ = _usualXStorageV0();
        // Calculate the raw amount of assets for the given shares
        uint256 assetsWithoutFee = convertToAssets(shares);

        // Calculate and subtract the withdrawal fee
        uint256 fee =
            Math.mulDiv(assetsWithoutFee, $.withdrawFeeBps, BASIS_POINT_BASE, Math.Rounding.Ceil);
 @>       assets = assetsWithoutFee - fee;
    }

e.g if user shares is 100, and fee is 5, maxAsset of user will return 95. So the first issue is that it reverts when user tries to claim total asset,

  // Check withdrawal limit
        uint256 maxAssets = maxWithdraw(owner);
 @>       if (assets > maxAssets) {
            revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
        }

Root Cause

https://github.com/sherlock-audit/2024-10-usual-labs-v1/blob/main/pegasus/packages/solidity/src/vaults/UsualX.sol#L336 Now then fee is calculated, it uses the asset amount that excludes fee to calculate fee.

 // Check withdrawal limit
        uint256 maxAssets = maxWithdraw(owner);
        if (assets > maxAssets) {
            revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
        }
        // Calculate shares needed
        shares = previewWithdraw(assets);
        uint256 fee = Math.mulDiv(assets, $.withdrawFeeBps, BASIS_POINT_BASE, Math.Rounding.Ceil); //@audit-issue

Note that since user cannot pass in more than 95 as asset to claim/withdraw, what is then used to calculate fee to be deducted is 95 instead of 100, so 4.75 is deducted in place of 5.

        // take the fee
        yieldStorage.totalDeposits -= fee;

Impact

5% lost of total fee

PoC

 function test_poc_Loss_of_Fee() public {
    uint256 fee = 500; // 5%
    vm.prank(admin);
    registryAccess.grantRole(WITHDRAW_FEE_UPDATER_ROLE, address(this));
    usualX.updateWithdrawFee(fee);

    uint256 depositAmount = 100e18;

    vm.startPrank(alice);
    ERC20Mock(usual).mint(alice, depositAmount);
    ERC20Mock(usual).approve(address(usualX), depositAmount);
    usualX.deposit(depositAmount, alice);

    uint256 snap = vm.snapshot();
    assertEq(usualX.totalAssets(), depositAmount); // Balance should be 100e18


    // Scenario: Alice redeems all her shares using `withdraw()`
    uint256 withdrawAssets = usualX.maxWithdraw(alice);
    uint256 withdrawShares = usualX.withdraw(withdrawAssets, alice, alice);
    assertEq(ERC20(usual).balanceOf(alice), 95e18); // 5% fee
    assertEq(usualX.balanceOf(alice), 0);
    assertEq(usualX.totalAssets(), 0); //Total assets should be zero since no reward credit yet
  }

LOGS:

Ran 1 test for test/vaults/UsualXUnit.t.sol:UsualXUnitTest
[FAIL. Reason: assertion failed: 250000000000000000 != 0] test_poc_Loss_of_Fee() (gas: 317579)

Mitigation

previewRedeem() can just return the actual asset and fee can then be calculated in withdraw()/redeem()