Attacker can set arbitrary allocator in case `cachedAllocator == address(0)`

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L204

Vulnerability details

Impact

The base interest allocator is set in the Pool through two steps: setBaseInterestAllocator() and confirmBaseInterestAllocator(). The confirmBaseInterestAllocator() function allows anyone to call it.

This function will execute the logic using the _newBaseInterestAllocator input passed in by the caller, instead of the value set by the owner in setBaseInterestAllocator(). The verification for _newBaseInterestAllocator == getPendingBaseInterestAllocator is only performed when cachedAllocator != address(0).

As a result, if cachedAllocator == address(0), anyone can set an arbitrary address for the allocator. And since the asset is approved to the new allocator address, attacker could steal all the fund available in the Pool contract.

Proof of Concept

function confirmBaseInterestAllocator(address _newBaseInterestAllocator) external {
    address cachedAllocator = getBaseInterestAllocator;
    // @audit in case cachedAllocator = address(0), anyone can set arbitrary allocator 
    if (cachedAllocator != address(0)) { 
        if (getPendingBaseInterestAllocatorSetTime + UPDATE_WAITING_TIME > block.timestamp) {
            revert TooSoonError();
        }
        if (getPendingBaseInterestAllocator != _newBaseInterestAllocator) {
            revert InvalidInputError();
        }
        IBaseInterestAllocator(cachedAllocator).transferAll();
        asset.approve(cachedAllocator, 0);
    }
    asset.approve(_newBaseInterestAllocator, type(uint256).max);

    getBaseInterestAllocator = _newBaseInterestAllocator;
    getPendingBaseInterestAllocator = address(0);
    getPendingBaseInterestAllocatorSetTime = type(uint256).max;

    emit BaseInterestAllocatorSet(_newBaseInterestAllocator);
}

As we can see, the check if input _newBaseInterestAllocator matches the pending value is only performed when cachedAllocator != address(0). The same applies to the UPDATE_WAITING_TIME check.

If the contract is deployed without an allocator set up, or whenever the owner sets the allocator address to 0, an attacker could call confirmBaseInterestAllocator() to set it to an arbitrary address.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider running all the checks when cachedAllocator == address(0) as well.

Assessed type

Invalid Validation

[c4-judge]

0xA5DF marked the issue as primary issue

[0xA5DF]

I have some doubts about severity, since this can happen only at the initial stage, and the owner can simply set this to the right address (owner would have to wait some time though) Leaving open for the sponsor to comment

[0xend]

This is definitely an issue but as pointed out, a low one.

[C4-Staff]

@0xend Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

And since the asset is approved to the new allocator address, attacker could steal all the fund available in the Pool contract.

Yeah, but when the allocator is changed the approval is revoked for the old allocator. As this happens only at the initial stage then no funds are expected to be in the pool at this time. Marking as low for now, but I'm open to hear arguments for med severity.

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moved to #70