KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions do not use whenNotPaused modifier

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L19-L21
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L183
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L243
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L215
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L269
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L184
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L200-L205
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L219
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L247
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L264-L269
https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L284

Vulnerability details

Impact

As shown by the code below, although PauseModifier is imported, the KangarooVault contract does not use the whenNotPaused modifier in any of its functions. More specifically, the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions do not use the whenNotPaused modifier.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L19-L21

import {PauseModifier} from "./utils/PauseModifier.sol";

contract KangarooVault is Auth, ReentrancyGuard, PauseModifier {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L183

    function initiateDeposit(address user, uint256 amount) external nonReentrant {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L243

    function processDepositQueue(uint256 idCount) external nonReentrant {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L215

    function initiateWithdrawal(address user, uint256 tokens) external nonReentrant {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L269

    function processWithdrawalQueue(uint256 idCount) external nonReentrant {

This is unlike the LiquidityPool contract; comparing to the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions, the LiquidityPool.deposit, LiquidityPool.queueDeposit, LiquidityPool.processDeposits, LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions have the similar functionalities but they all use the whenNotPaused modifier. As a result, when an emergency, such as a hack, occurs, the protocol can pause the LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions to prevent or reduce damages, such as preventing users and the protocol from losing funds, but cannot do that for the KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L184

    function deposit(uint256 amount, address user) external override nonReentrant whenNotPaused("POOL_DEPOSIT") {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L200-L205

    function queueDeposit(uint256 amount, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_DEPOSIT")
    {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L219

    function processDeposits(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_DEPOSITS") {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L247

    function withdraw(uint256 tokens, address user) external override nonReentrant whenNotPaused("POOL_WITHDRAW") {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L264-L269

    function queueWithdraw(uint256 tokens, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_WITHDRAW")
    {

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L284

    function processWithdraws(uint256 count) external override nonReentrant whenNotPaused("POOL_PROCESS_WITHDRAWS") {

Proof of Concept

The following steps can occur for the described scenario.

1. An emergency, such as a hack, occurs in which further withdrawals can cause users and the protocol to lose funds.
2. The protocol team is able to pause the LiquidityPool.withdraw, LiquidityPool.queueWithdraw, and LiquidityPool.processWithdraws functions.
3. However, the protocol team is unable to pause the KangarooVault.initiateWithdrawal and KangarooVault.processWithdrawalQueue functions.
4. As a result, funds can be lost from the KangarooVault.

Tools Used

VSCode

Recommended Mitigation Steps

The KangarooVault.initiateDeposit, KangarooVault.processDepositQueue, KangarooVault.initiateWithdrawal, and KangarooVault.processWithdrawalQueue functions can be updated to use the whenNotPaused modifier.

[c4-judge]

JustDravee marked the issue as primary issue

[c4-sponsor]

rivalq marked the issue as disagree with severity

[JustDravee]

rivalq marked the issue as disagree with severity

Not such an easy one to grade. Historically, missing whenNotPaused modifiers are considered as a medium risk issue:

• Missing whenNotPaused modifier 2022-06-connext-findings#175
• StakingRewards.sol#stake is intended to be pausable but isn't 2022-09-y2k-finance-findings#38

The only time they are considered as Low severity findings are when they're actually put on too many functions and should be removed:

• https://code4rena.com/reports/2022-02-jpyc#l-04-fiattokenv1--v2-remove-whennotpaused-modifier-from-cancelauthorization-and-decreaseallowance-functions
• https://code4rena.com/reports/2023-01-drips#l-04-collecting-funds-should-be-usable-while-the-dripshub-contract-is-paused

Therefore I believe that Medium Severity is the right risk level, at least for this type of issue on code4rena and for the time being

[mubaris]

Medium severity seems fair

[c4-sponsor]

mubaris marked the issue as sponsor confirmed

[c4-judge]

JustDravee marked the issue as selected for report