Rage trade junior vault deposit is subject to deposit cap restriction

Summary

Rage trade junior vault deposit is subject to deposit cap

Vulnerability Detail

In the current implementation,

the code integrate with the deposit and redeem and withdraw function

contract DNGMXVaultController is IController {

    /* -------------------------------------------------------------------------- */
    /*                             CONSTANT VARIABLES                             */
    /* -------------------------------------------------------------------------- */

    /// @notice depositToken(address,address,uint256)
    bytes4 constant DEPOSIT = 0xfb0f97a8;

    /// @notice redeemToken(address,address,uint256)
    bytes4 constant REDEEM = 0x0d71bdc3;

    /// @notice withdrawToken(address,address,uint256)
    bytes4 constant WITHDRAW = 0x01e33667;

However, the junior vault of the rage trade is subject to a deposit cap set by admin.

https://github.com/RageTrade/delta-neutral-gmx-vaults/blob/a2107d37b789494454bd4ede7d217d8723474de4/contracts/vaults/DnGmxJuniorVault.sol#L183

/// @notice set admin paramters
/// @param newKeeper keeper address
/// @param dnGmxSeniorVault senior vault address
/// @param newDepositCap deposit cap
/// @param batchingManager batching manager (responsible for staking tokens into GMX)
/// @param withdrawFeeBps fees bps on withdrawals and redeems
function setAdminParams(
    address newKeeper,
    address dnGmxSeniorVault,
    uint256 newDepositCap,
    address batchingManager,
    uint16 withdrawFeeBps,
    uint24 feeTierWethWbtcPool
) external onlyOwner {
    if (withdrawFeeBps > MAX_BPS) revert InvalidWithdrawFeeBps();

    state.keeper = newKeeper;
    state.depositCap = newDepositCap;
    state.withdrawFeeBps = withdrawFeeBps;
    state.feeTierWethWbtcPool = feeTierWethWbtcPool;

    state.dnGmxSeniorVault = IDnGmxSeniorVault(dnGmxSeniorVault);
    state.batchingManager = IDnGmxBatchingManager(batchingManager);

    emit AdminParamsUpdated(newKeeper, dnGmxSeniorVault, newDepositCap, batchingManager, withdrawFeeBps);
}

Such parameter is to limit to the deposit

/// @notice returns maximum amount of shares that a user can deposit
/// @return maximum asset amount
function maxDeposit(address) public view override(IERC4626, ERC4626Upgradeable) returns (uint256) {
    return state.depositCap - state.totalAssets(true);
}

Impact

When the admin on rage trade adjust the deposit cap or the deposit cap on rage trade exceeds the max deposit cap, deposit on the sentiment can revert.

Code Snippet

https://github.com/sherlock-audit/2023-01-sentiment/blob/main/controller-52/src/rage/DNGMXVaultController.sol#L51

Tool used

Manual Review

Recommendation

We recommend the protocol call preview function first to calculate how many shares can be minted before calling deposit and also check the deposit cap on rage trade side instead of just validating the deposit signature and return the can call as tru.

r0ohafza commented 1 year ago

Disagree with severity since the PoC does not identify how a deposit cap could be viewed as an attack. The revert can be prevented by doing some off chain checks before trying to deposit.

hrishibhat commented 1 year ago

Agree with Sponsor, Worst case a deposit reverts because of the deposit cap set by a GMX admin. Considering this a low.

sherlock-audit / 2023-01-sentiment-judging

ctf_sec - Rage trade junior vault deposit is subject to deposit cap restriction #35