PoolManager can always bypass the check which make sure to withdraw funds from pool, allocation should be ended and 30 days have passed.

Vulnerability Detail

PoolManager can withdraw funds from the pool via

File: contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol

394   function withdraw(uint256 _amount) external onlyPoolManager(msg.sender) {
395        if (block.timestamp <= allocationEndTime + 30 days) {
            revert INVALID();
        }

        IAllo.Pool memory pool = allo.getPool(poolId);

        if (_amount > poolAmount) {
            revert INVALID();
        }

        poolAmount -= _amount;

        // Transfer the tokens to the 'msg.sender' (pool manager calling function)
        _transferAmount(pool.token, msg.sender, _amount);
409    }

https://github.com/allo-protocol/allo-v2/blob/0b881ef4a0013d2809374c9ea69f4cf1288dfe62/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L394 At line number 395 it checks that PoolManager can only withdraw if allocation has ended and 30 days have passed but he can always bypass this check to withdraw funds by manipulating updatePoolTimestamps

File: contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol
 function updatePoolTimestamps(
        uint64 _registrationStartTime,
        uint64 _registrationEndTime,
        uint64 _allocationStartTime,
        uint64 _allocationEndTime
    ) external onlyPoolManager(msg.sender) {
        // If the timestamps are invalid this will revert - See details in '_isPoolTimestampValid'
        _isPoolTimestampValid(_registrationStartTime, _registrationEndTime, _allocationStartTime, _allocationEndTime);

        // Set the updated timestamps
        registrationStartTime = _registrationStartTime;
        registrationEndTime = _registrationEndTime;
        allocationStartTime = _allocationStartTime;
        allocationEndTime = _allocationEndTime;

        // Emit that the timestamps have been updated with the updated values
        emit TimestampsUpdated(
            registrationStartTime, registrationEndTime, allocationStartTime, allocationEndTime, msg.sender
        );
    }

https://github.com/allo-protocol/allo-v2/blob/0b881ef4a0013d2809374c9ea69f4cf1288dfe62/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L369C4-L388C6 by changing allocation time he can withdraw funds. It was advised that PoolManager is trusted but he shouldn't have this much power to withdraw funds by bypassing this check

Impact

PoolManager can bypass a important check which shouldn't

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/0b881ef4a0013d2809374c9ea69f4cf1288dfe62/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L369C4-L388C6

https://github.com/allo-protocol/allo-v2/blob/0b881ef4a0013d2809374c9ea69f4cf1288dfe62/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L394

Tool used

Manual Review

Recommendation

Need some Modification and I will need to discuss some things to propose a mitigation

sherlock-audit / 2023-09-Gitcoin-judging