Popcorn contest - peakbolt's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L138 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L174 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L215 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L257

Vulnerability details

Impact

In Vault.sol, the syncFeeCheckpoint modifier sets the highWaterMark to share price, which affects accruedPerformanceFee(), resulting in loss of fee for the owner.

A performance fee can be taken by the vault owner (shareValue - highWaterMark), when shareValue > highWaterMark. However, Vault.deposit(), Vault.mint() and Vault.withdraw() have syncFeeCheckpoint modifiers, which will reset the highWaterMark to share price before the performance fee is taken. So if deposit/mint/withdraw is performed after the vault share price increased, the vault owner will lose the performance fee and be shortchanged.

Note: redeem() does not have this issue as it is somehow missing the syncFeeCheckpoint.

modifier syncFeeCheckpoint() {
    _;
    highWaterMark = convertToAssets(1e18);
}

function accruedPerformanceFee() public view returns (uint256) {
    uint256 highWaterMark_ = highWaterMark;
    uint256 shareValue = convertToAssets(1e18);
    uint256 performanceFee = fees.performance;

    return
        performanceFee > 0 && shareValue > highWaterMark
            ? performanceFee.mulDiv(
                (shareValue - highWaterMark) * totalSupply(),
                1e36,
                Math.Rounding.Down
            )
            : 0;
}

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L138 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L174 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L215 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L257

Recommended Mitigation Steps

Remove SyncFeeCheckpoint modifier.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L162 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L232

Vulnerability details

Impact

In AdapterBase.sol, the harvest() does not update lastHarvest after the harvest delegatecall to strategy. Due to that, the harvest cool down check does not have any effect as it will always be true and trigger the delegate call to strategy.

This will cause every _deposit() and _withdraw() calls to waste gas and tokens as they will call harvest() at the end.

function harvest() public takeFees {
    if (
        address(strategy) != address(0) &&
        ((lastHarvest + harvestCooldown) < block.timestamp)
    ) {
        // solhint-disable
        address(strategy).delegatecall(
            abi.encodeWithSignature("harvest()")
        );
    }

    emit Harvested();
}

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L162 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L232

Recommended Mitigation Steps

Update the lastHarvest upon successful harvest call.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L434 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

Vulnerability details

Impact

In Vault.sol, accruedManagementFee() will compute the wrong management fee amount when the management fee is changed from zero to non-zero value.

As feesUpdatedAt is not updated in changeFees(), the management fee will be computed from the time of vault deployment, instead of new fee effective time.

A malicious vault owner could create a vault with zero management fee and increase it after a long period, taking it more fees than the user expected, and then withdraw the fees. Even though there is a quit period, users are likely to stay if the fees are reasonable as they are unaware of the management fee miscalculation.

Note: even if the management fee is initially non-zero, the fee calculation could still be miscalculated and backdated with the new fee settings. This is because feesUpdatedAt is updated during fee accrual, which could be before the new fee takes effect.

function accruedManagementFee() public view returns (uint256) {
    uint256 managementFee = fees.management;
    return
        managementFee > 0
            ? managementFee.mulDiv(
                totalAssets() * (block.timestamp - feesUpdatedAt),
                SECONDS_PER_YEAR,
                Math.Rounding.Down
            ) / 1e18
            : 0;
}

function changeFees() external {
    if (block.timestamp < proposedFeeTime + quitPeriod)
        revert NotPassedQuitPeriod(quitPeriod);

    emit ChangedFees(fees, proposedFees);
    fees = proposedFees;
}

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L434 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

Proof of Concept

Add this test case to Vault.t.sol

function test__managementFee_peakbolt() public {

	//start with zero fees
	_setFees(0, 0, 0, 0);

	//alice deposit 1000 ETH
	uint256 depositAmount = 1000 ether;
	asset.mint(alice, depositAmount);
	vm.startPrank(alice);
	asset.approve(address(vault), depositAmount);
	vault.deposit(depositAmount, alice);
	vm.stopPrank();

	// set 10% mgmt fee after 1 year, take effect after 3 day quit period
	vm.roll(block.timestamp + 365.25 days);
	_setFees(0, 0, 1e17, 0);

	// fee recipient balance should be zero as no fee collected so far
	assertEq(vault.balanceOf(feeRecipient),0);

	// vault total supply should be same as deposit amount as no shares minted for fees
	assertEq(vault.totalSupply(), depositAmount);

	// take management fee, which will be higher than expected
	vault.takeManagementAndPerformanceFees();

	// vault.balanceOf(feeRecipient) expected to be zero as management fee just took effect. 
	// however, this will fail as vault.accruedManagementFee() compute the fee from the vault creation date.
	assertEq(vault.balanceOf(feeRecipient), 0);
}

Recommended Mitigation Steps

In changeFees(), add takeFees modifier to compute the accrued fees before new fee take effect, it will also set feesUpdatedAt = block.timestamp.