jasonxiale - `Vault._state.liquidity.totalDeposit` can avoid being decreased.

[sherlock-admin3]

jasonxiale

medium

Vault._state.liquidity.totalDeposit can avoid being decreased.

Summary

Vault._state.liquidity.totalDeposit is used as total deposit amount of baseToken in Vault.sol, when user deposits into Vault.sol, it will be increased, and when users withdraws it will be decreased. In current implementation, there is a code path that a user can withdraw token from the vault without decreasing Vault._state.liquidity.totalDeposit.

Vulnerability Detail

When user calls Vault.redeem, the function will transfer ERC20 token to the caller. And when the user calls Vault.initiateWithdraw, Vault._initiateWithdraw will update _state.liquidity.totalDeposit

In Vault.sol#L507, withdrawDepositEquivalent is calculated as uint256 withdrawDepositEquivalent = (userDeposits * shares) / userShares;

And in Vault.sol#L502, userDeposits is assigned by depositReceipt.cumulativeAmount So if depositReceipt.cumulativeAmount is 0, withdrawDepositEquivalent will be 0, and _state.liquidity.totalDeposit will keep unchanged after Vault.sol#L509

It's doable by:

1. Alice call Vault.redeem to withdraw some token, during Vault._redeem, some ERC20 will be transferred to Alice
2. Alice transfers the ERC20 token to Bob(who is never interact with the Vault before)
3. after the epoch, Bob will calls Vault.initiateWithdraw to withdraw the baseToken, in such case, because of depositReceipt is zero, userDepositswill be zero, and leaving _state.liquidity.totalDeposit unchanged.
4. Bob calls Vault.completeWithdraw to withdraw the token

After the call, Bob will receives all the asset he deserves, but leaving _state.liquidity.totalDeposit unchanged.

For POC, add the following code to unit/Vault.user.t.sol and run forge test --mc VaultUserTest --mt testTransferCompleteWithdraw -vv

    function testTransferCompleteWithdraw() public {
        uint256 totalDeposit     = 10000*1e6;
        uint256 sharesToWithdraw = 5000*1e6;
        uint256 sideTokenPrice   = 1e18;

        address alice = address(0x1111);
        address bob   = address(0x2222);

        // User deposit (and later withdraw) a given amount
        vm.prank(admin);
        baseToken.mint(alice, sharesToWithdraw);

        vm.startPrank(alice);
        baseToken.approve(address(vault), sharesToWithdraw);
        vault.deposit(sharesToWithdraw, alice, 0);
        vm.stopPrank();

        vm.warp(vault.getEpoch().current + 1);
        vm.prank(admin);
        vault.rollEpoch();

        vm.prank(alice);
        vault.redeem(sharesToWithdraw);

        vm.prank(alice);
        vault.transfer(bob, sharesToWithdraw);

        {
            (, , uint256 pendingWithdrawals, , uint256 totalVaultDeposit, uint256 heldShares, uint256 newHeldShares, , ) = vault.vaultState();
            console2.log("totalVaultDeposit                     :", totalVaultDeposit);
            console2.log("baseToken.balanceOf(bob)              :", baseToken.balanceOf(bob));
        }

        vm.prank(bob);
        vault.initiateWithdraw(sharesToWithdraw);

        vm.prank(admin);
        priceOracle.setTokenPrice(address(sideToken), sideTokenPrice);

        vm.warp(vault.getEpoch().current + 1);
        vm.prank(admin);
        vault.rollEpoch();

        uint256 sharePrice = vault.epochPricePerShare(vault.getEpoch().previous);

        vm.prank(bob);
        vault.completeWithdraw();

        {
            (, , uint256 pendingWithdrawals, , uint256 totalVaultDeposit, uint256 heldShares, uint256 newHeldShares, , ) = vault.vaultState();
            console2.log("totalVaultDeposit                     :", totalVaultDeposit);
            console2.log("baseToken.balanceOf(bob)              :", baseToken.balanceOf(bob));
        }

    }

Impact

By abusing this method, Vault will handle less baseToken than expected

Code Snippet

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/3241f1bf0c8e951a41dd2e51997f64ef3ec017bd/smilee-v2-contracts/src/Vault.sol#L500-L510

Tool used

Manual Review

Recommendation

Maybe we can revert in Vault.transfer/transferFrom

Duplicate of #39

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

panprog commented:

valid medium, dup of #39

takarez commented:

valid; medium(10)

[metadato-eth]

SAME AS 39