code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The Vault contract syncs the highWaterMark on deposit, mint and withdraw, which can be used to avoid paying the performance fees, by performing any of these actions with a tiny amount. After Clarification with the sponsor, this was caused by a merge gone wrong and is not supposed to be there

Proof of Concept

The highWaterMark is used as a threshold to trigger the payment of a performance fee:

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;
}

The Vault contains the following modifier used for deposits, mints and withdraws:

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

This modifier can be abused by performing tiny deposits, mints and withdraws to move the highWaterMark to current share values to avoid the triggering of the performance fee.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the syncFeeCheckpoint modifier and all its usages from Vault

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #30

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed