ctf_sec - Deposit and withdraw and redeem can be blocked with Rage trade junior vault integration when the rage trade vault contract is paused.

[github-actions[bot]]

ctf_sec

medium

Deposit and withdraw and redeem can be blocked with Rage trade junior vault integration when the rage trade vault contract is paused.

Summary

Deposit and withdraw and redeem can be blocked with Rage trade junior vault integration when the rage trade vault contract is paused.

Vulnerability Detail

In the current integration, the code integrate with Rage trade junior vault:

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 rage trade admin can pause the deposit, mint, withdraw and redeem

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

/// @notice pause deposit, mint, withdraw and redeem
function pause() external onlyOwner {
    _pause();
}

/// @notice unpause deposit, mint, withdraw and redeem
function unpause() external onlyOwner {
    _unpause();
}

Impact

Deposit and withdraw and redeem can be blocked with Rage trade junior vault integration when the rage trade vault contract is paused.

Code Snippet

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

Tool used

Manual Review

Recommendation

The canCall method for rage trade integration should not only the check the signature but also check if the contract is paused.

[r0ohafza]

Looking at this issue I see that liquidations will be impacted the most due to the pause state. Deposits reverting due to a paused contract does not lead to any form of attack so adding an extra check on chain to the controller is not required. Would love to hear @zobront thoughts on the liquidations risk this could cause.

[r0ohafza]

Since rage trade was audited by sherlock, would also like to here what the sherlock team thinks about this particular issue as an integration to sentiment. cc: @hrishibhat

[zobront]

Looking at this issue I see that liquidations will be impacted the most due to the pause state. Deposits reverting due to a paused contract does not lead to any form of attack so adding an extra check on chain to the controller is not required. Would love to hear @zobront thoughts on the liquidations risk this could cause.

I don't see any risks to this. All they're saying is that the call could revert, but if it does, the function will revert anyways and none of the Controller checks matter. I think this is a misunderstanding of when/how the Controller is called and what types of risks we're looking for in this system.

[hrishibhat]

Agree with Lead Watson's comment. Don't really see any other issue here.