Owner cannot change the allocator address from `address(0)`

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The base interest allocator is set in the Pool through two steps: setBaseInterestAllocator() and confirmBaseInterestAllocator(). The confirmBaseInterestAllocator() function can be called by anyone.

This function executes the logic using the _newBaseInterestAllocator input from the caller, not the value set by the owner in setBaseInterestAllocator(). The check for _newBaseInterestAllocator == getPendingBaseInterestAllocator is only done when cachedAllocator != address(0).

After all, the pending values are reset. getPendingBaseInterestAllocator is reset to address(0) and getPendingBaseInterestAllocatorSetTime is reset to type(uint256).max. An attacker could exploit this to prevent the owner from changing the allocator address from address(0).

Proof of Concept

function confirmBaseInterestAllocator(address _newBaseInterestAllocator) external {
    address cachedAllocator = getBaseInterestAllocator;
    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);
}

When the owner tries to change the allocator address from an address(0) allocator, they need to call setBaseInterestAllocator(). This function records the request in getPendingBaseInterestAllocator and getPendingBaseInterestAllocatorSetTime.

If the attacker immediately calls the confirmBaseInterestAllocator() function with _newBaseInterestAllocator = address(0), the getPendingBaseInterestAllocator will reset, preventing the owner from ever setting a new base interest allocator.

Tools Used

Manual Review

Recommended Mitigation Steps

Only allow owner to call confirmBaseInterestAllocator().

Assessed type

DoS

[0xA5DF]

If the attacker immediately calls the confirmBaseInterestAllocator() function with _newBaseInterestAllocator = address(0), the getPendingBaseInterestAllocator will reset, preventing the owner from ever setting a new base interest allocator.

If the attacker calls confirmBaseInterestAllocator() with the zero address then the owner can call this again with the right address. If the attacker calls this with anything else then the owner can call again setBaseInterestAllocator() and then confirmBaseInterestAllocator()

Tending to mark this as low severity

[c4-judge]

0xA5DF marked the issue as primary issue

[0xend]

Low sev

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Marking as low due to my explanation above

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moved to #70