KangarooVault.sol : anyone can call the `processWithdrawalQueue`. This would hurt the user when the token price is low

[code423n4]

Lines of code

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

Vulnerability details

Impact

When the price is low, user would get less amount.

Proof of Concept

processWithdrawalQueue can be called by any one.

  function processWithdrawalQueue(uint256 idCount) external nonReentrant {
    for (uint256 i = 0; i < idCount; i++) {
        uint256 tokenPrice = getTokenPrice();

        QueuedWithdraw storage current = withdrawalQueue[queuedWithdrawalHead];

        if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minWithdrawDelay) {
            return;
        }

        uint256 availableFunds = totalFunds - usedFunds;

        if (availableFunds == 0) {
            return;
        }

        uint256 susdToReturn = current.withdrawnTokens.mulWadDown(tokenPrice);

        // Partial withdrawals if not enough available funds in the vault
        // Queue head is not increased
        if (susdToReturn > availableFunds) {
            current.returnedAmount = availableFunds;
            uint256 tokensBurned = availableFunds.divWadUp(tokenPrice);

            totalQueuedWithdrawals -= tokensBurned;
            current.withdrawnTokens -= tokensBurned;

            totalFunds -= availableFunds;

            if (withdrawalFee > 0) {
                uint256 withdrawFees = availableFunds.mulWadDown(withdrawalFee);
                SUSD.safeTransfer(feeReceipient, withdrawFees);
                availableFunds -= withdrawFees;
            }

            SUSD.safeTransfer(current.user, availableFunds);

            emit ProcessWithdrawalPartially(
                current.id, current.user, tokensBurned, availableFunds, current.requestedTime
                );
            return;
        } else {
            current.returnedAmount = susdToReturn;
            totalQueuedWithdrawals -= current.withdrawnTokens;

            totalFunds -= susdToReturn;

            if (withdrawalFee > 0) {
                uint256 withdrawFees = susdToReturn.mulWadDown(withdrawalFee);
                SUSD.safeTransfer(feeReceipient, withdrawFees);
                susdToReturn -= withdrawFees;
            }

            SUSD.safeTransfer(current.user, susdToReturn);

            emit ProcessWithdrawal(
                current.id, current.user, current.withdrawnTokens, susdToReturn, current.requestedTime
                );

            current.withdrawnTokens = 0;
        }

        queuedWithdrawalHead++;
    }
}

Tools Used

Recommended Mitigation Steps

[JustDravee]

Seems invalid but will talk about it with the sponsor, as this could very well be a feature, and a withdraw should probably be expected after the delay. Lack of mitigation also decreases the quality (would've been easy writing a require statement with current.user == msg.sender) Will mark as invalid for now and change later, if relevant.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[aktech297]

Apologies for incomplete content. When I was writing about this issue, the time was over , so not able to provide full details. I will come back for discussion during QA.

[aktech297]

@JustDravee

Thanks for your feedback.

I believe that this issue should be something to reconsider. Though there is some time delay for withdrawal, this will not guarantee that the price is up and desirable always to withdraw.

My thought towards the undesirable event, when the price is fluctuating the actual user will not call this function as this would result in loss.

lets assume, when the user has 10000000 and there some fluctuation in price lets say 5%, then calling withdrawal this function could result in loss of 500,000. calling this function by anybody would result this loss to the actual user.

So, allowing the actual user (in this case current.user == msg.sender) will be the good implementation that does not result in loss of funds.

[mubaris]

This is a design choice and it has come up multiple times in other issues. We are actually using this mechanism in our Earn v2 vaults and Lyra's Option Liquidity Pools also use the same mechanism without a slippage implementation. If vault loses money, this will be shared equally among all holders and it is not necessarily because of this implementation

[JustDravee]

Will keep as such

[aktech297]

would you please add some comments why this is not an issue ? why the loss of funds is not a concern which is intentionally caused by the user who is external. ? Its is not single user who is gonna affect, everybody who relies on particular asset will be impacted. If so, how the loss can be shared. Practically not feasible. does the user unnecessarily need to bear this loss ?

lets consider one scenario,

when 10 persons deposited asset worth each 1000.

Total 10000.

when there is 30% fluctuation in span of 5 days. At the end of five days, the total asset value of all 10 persons would be 7000.

when somebody who is not the actual user is making the call, then the transaction would be done with 700 to each person.

The loss for each personal would be 300.

imo, it is not fair to bear the loss which is not necessary supposed to be. limo, this design could be serious cause of concern.

If the team wants to stick to this, better this can be clearly mentioned in warning.

I kindly request @JustDravee to take a look at this with C4 guidelines where the loss of funds is treated with high concern always.

[mubaris]

Tell me why how this specific code is leading to loss of funds.

If token price is low, user would lose funds. But any of the code you mentioned is leading to low token prices

[aktech297]

Kindly refer the getTokePrice() from above code snippet.

[JustDravee]

Thank you @aktech297 for your input regarding the finding's severity assessment. However, I believe that the attack scenario you described regarding the potential loss of assets due to the price becoming low is inherent to this type of system and not a vulnerability. When looking at the documentation, this falls under an "indirect attack", and the hypotheticals mentioned (like 30% drop) can actually be considered a hand-wavy hypothetical:

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

The alternative design could've been a pool for withdrawals but it comes with its own unfair part (first come first serve or might not be able to withdraw).

As for medium severity, I believe the protocol's function or availability aren't impacted:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

As for QA, the state is handled as designed and the function works as intended:

Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments)

I think it's important to keep in mind that the focus here is on actual vulnerabilities and not just pointing out design choices.

I hope this answer clarifies better why this finding was invalidated.

[aktech297]

Hi @JustDravee

Thanks for your response.

Unfortunately I can not agree on the point that it is system design.

This issue is similar to access control permission issues. where the lack of access control could lead to loss funds either in the form of transferring the funds to one who is caller or transferring the funds to some dark location.

I am aware that the lack of access control which leads to loss of funds is treated as valid issue.

It falls under the actual vulnerability imo. Since it is crypto, fluctuation in the range of 30% is possible. so, it can not be hand wavy hypothetical.

one of the scenario where the huge price fluctuation happen is flash loan attack. somebody would make flash loan attach and cause the price to fall and call these functions that do not have proper access control.

Audit is the place where, in addition to finding the bugs that are obvious in the code also deciphering the issues that could be exploited by using the poor system design. So, flagging those system poor design is also part of audit work I think.

lets review one of the issue https://github.com/code-423n4/2023-03-polynomial-findings/issues/226

the root cause of the issue is market sell-off that causes the collateral to be worth much less than before.. though it is called by the valid user, still it leads to loss of funds and hence considered as valid issue.

Thanks!

[JustDravee]

While we can respect your opinion and effort, this is still something we don't agree on. Your arguments may feel sound to you, but they really, in fact, aren't.

[aktech297]

No prob