Popcorn contest - thecatking's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L429-L450

Vulnerability details

Impact

The harvest() function can be spammed due to lack of checks, creating a grieving attack that would waste the vault's/adapter's tokens, which is one of the attack vectors specifically mentioned in the contest.

The strategies in the Popcorn system utilize harvesting rewards as a means of increasing their value. In fact, the dedicated harvest() is available to be called publicly to perform whatever logic it needs through a delegateCall to the strategy contract. However, while it is intended to be used with a check through a lastHarvest variable that is intended to hold the time of the last harvest, that check is missing, and the lastHarvest variable is entirely unused.

This creates a possibility to call the harvest() function at any point by malicious actors (since there are also no access modifiers either), which, if significant fees for swaps/entering pools are involved, could be not just a grieving attack, but an exploit that could be used by a frontrunner. For example, if a harvest involves rebalancing from one token to another, this could be called an infinite number of times, until all vault's assets are emptied into fees from the rebalancing.

Coupled with another issue with vault.changeAdapter() redeeming and depositing the vault's entire asset balance due to a lack of check on adapter==proposedAdapter, the lack of this check on harvest() will create really significant leaks of funds through fees.

Proof of Concept

Please find below the code mentioned above: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L429-L450

Tools Used

Manual audit.

Recommended Mitigation Steps

Add the check to see if enough time has passed since the last harvest, utilizing the lastHarvest and harvestCooldown variables.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L594-L613

Vulnerability details

Impact

Due to custom vault/strategy nature of this project, the impact could vary vastly and go up to critical, including significant (and potentially even total) loss of funds. The code in question has several consequences that this submission will focus on. Let's review the issue first.

The changeAdapter() function is intended to be called by anyone to move vault to a new adapter from the adapter address to the proposedAdapter address. However, while there is a check to see if enough time has passed since the new adapter has been proposed (block.timestamp < proposedAdapterTime + quitPeriod), there is no check to see if adapter==proposedAdapter, and adapter gets set to be proposedAdapter again and again along with the rest of the logic. In other words, the entirety of the function will go through its whole logic even if there is no change pending to the adapter address. A simple check to see if they are the same address would mitigate any issues. However, as is, there are unfortunately several consequences:

1. Event spam of ChangedAdapter event. Very minor issue.
2. takeFees modifier that causes the mint and subsequent transfer of shares to the feeRecipient will be called as many time as malicious actors would like. This is particularly bad because the changeAdapter() lacks a reentrancy protection, while the only other function with the takeFees modifier - takeManagementAndPerformanceFees() has the nonReentrant modifier on top of it. An auxiliary issue will be filed just to highlight the lack of nonReentrant modifier on the changeAdapter() function since the resolution will be different from this issue's. Please note that the adapter functions discussed below have a separate nonReentrant modifier on them, so the impact for this would be contained in actions that a malicious actor could do with fee movement.
3. The rest of this function's logic has to do with redeeming, removing approvals, approving, and depositing assets to the adapter from the vault, essentially juggling assets back and forth with the same address if adapter=proposedAdapter.

#1 is minor and self-explanatory. However, #2 and #3 vastly depend on the implementations of these custom vaults, adapters, and strategies. Still, #2 spam of a potentially re-entrant function is a fairly dangerous security opening, especially since takeFees seems to be intended to be used by the team with nonReentrant as is evident by the takeManagementAndPerformanceFees() function and its modifiers. The rest of this issue will focus on #3's impact as it goes a bit deeper into the adapter code.

As noted above, changeAdapter() calls into the adapter contract both before and after setting it to the new proposedAdapter address, which in this case is of course the same contract. Namely, it calls adapter.redeem() and adapter.deposit(). Both of these are functions crucial to the whole system that effectively "start" and "end" the lifecycle of adapters. Due to DeFi nature of this project, the deposits and redeems may have non-trivial fees associated with them, depending on what the strategies that these adapters use are. Moreover, the adapters explicitly have maxMint() and maxDeposit() checking functions (that are meant to be overridden) to see if the minter/depositor are over their limits. In this particular case, the minter/depositor is going to be the vault contract itself, and it will be minting/depositing very large sums of assets -- the vault's whole balance in fact. While it may be that mint/deposit counters that get checked in maxMint() and maxDeposit() functions get reset during the redeem/deposit cycle we find ourselves in, there is a significant chance that may not be the case. Namely, a intentional gated rollout of strategies (which seems to be an intended use case by the comments in this section) might only allow whitelisted address (like the vault) to deposit once with no account for redeems. This would break that use case. Additionally, a faulty implementation of an adapter/strategy that simply forgets to reset these counters or that gets broken by this constant juggle of assets would, of course, also break.

Finally, and perhaps most crucially, the redeem() and deposit() functions call into the harvest() function in the adapter, meaning the harvest will be spammed as well with the entirety of the vault's assets. While one may argue that the harvests won't happen due to a check that is supposed to happen with the lastHarvest timestamp, unfortunately, this check is missing, and another issue will be filed for that. Assuming the strategies associated with harvest(), redeem(), and deposit() are anything like common DeFi strategies, it may involve swapping and interacting with other smart contracts in the ecosystem. Also, note the now familiar takeFees modifier on harvest()! A potential source of pain in this scenario as well, although it doesn't seem to be exploitable based on the implementation given in this contract.

Putting all of this together, we get an ability to send the vault's entire asset balance through the redeem - harvest - deposit - harvest cycle as many times as the malicious actor would want. With strategies that have any slippage/fees at all that can be frontrun for the above actions, this could lead to a complete asset drain, which would be disastrous. Again, this would all depend on the actual implementation of these vaults, adapters, and strategies, but having swapping/entering pools with fees is very common for yield strategies in the DeFi space, so this would very likely be exploited.

Proof of Concept

Please find below the code referenced above in the rough order it was mentioned: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L594-L613 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L472-L494 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L193-L204 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L221-L232 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L110-L122 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L155-L162 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L399-L421 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L433-L450

Tools Used

Manual auditing.

Recommended Mitigation Steps

Thankfully, for this specific issue, there is a very simple mitigation step -- simply add a require check to see if adapter==proposedAdapter. This would prevent moving the whole vault balance back and forth with the adapter and all the misfortunes associated with that cycle being repeated from happening.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L594-L613

Vulnerability details

Impact

The changeAdapter() function through its modifier takeFees ends up transferring funds (mint of shares), which may be an issue that creates a reentrancy that would create a significant exploit.

As explained in depth in another issue submitted about the changeAdapter() function, it may be spammed by malicious actors because there is no check if the adapter already got set to the proposedAdapter. On top of the deeper consequences of that, this function also calls the takeFees modifier which mints shares to the fee recipient. As is evident by the only other function with the takeFees modifier -- takeManagementAndPerformanceFees(), a minimal function made specifically to call that modifier -- the developer intention is clearly to protect against reentrancies associated with asset transfers in fee taking. Upon a first look, it does not seem like this could be significantly exploited through takeFees, though it is best to guard against reentrancies as intended, especially due to the customizable nature of vaults where a fork/version of them may in fact suffer significantly from reentrancy that could be caused by this changeAdapter() function.

It is hard to pinpoint how damage may be caused without assuming some worst case scenarios that the attacker may come up with, so, even though this is a reentrancy related issue, medium impact seems more appropriate than high.

Proof of Concept

Please find below the code mentioned: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L594 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L472-L494

Tools Used

Manual auditing.

Recommended Mitigation Steps

Place a reentrancy protection guard nonReentrant on the changeAdapter() function.