code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L34-L55

Vulnerability details

Impact

Yearn Finance vaults may sometimes be migrated to newer contracts with different fee structures or strategies. When this occurs the previous vault is disabled, where deposits are no longer allowed. Withdrawals are intended to be enabled but not deposits, and sometimes those fail as well. When this occurs users need to migrate funds from the old vault to the new vault otherwise they will no longer accrue any interest on their existing deposits. The existing YearnAdapter.sol does not consider this, and has no migration capabilities. If vault deprecation occurs then any deployed Yearn Adapters will no longer accrue any interest permanently.

Similarly, if the existing vault is deprecated in an emergency due to security vulnerabilities, then the inability to migrate might place those funds at risk as well.

Proof of Concept

Information about Yearn Vault migrations can be found below

Tools Used

Code Editor Github

Recommended Mitigation Steps

Add the following function to YearnAdapter.sol

/// @dev Verify that current Yearn vault is latest with Yearn registry. If not, migrate funds automatically
function migrate() external {
    address newVault = registry.latestVault(token);
    // Check if active yVault is latest yVault
    if(newVault != address(yVault)) {
        // If it is not, migrate the assets to the new yVault
        IERC20 tokenContract = IERC20(token);

        // Update storage
        VaultAPI oldVault = yVault;
        yVault = VaultAPI(newVault);

        // Withdraw all assets from old vault
        uint assets = oldVault.withdraw(type(uint).max);
        // Approve deposits to new yVault
        tokenContract.safeApprove(newVault, assets);

        // Redeposit assets into target vault
        yVault.deposit(assets);
    }  
}

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)