Privileged addresses can be transferred without confirmation even to invalid values

[ggrieco-tob]

Severity: Low Difficulty: Medium

Description

An incorrect use of the functions to set privileged addresses in contracts can irreversibly set them to invalid addresses, such as 0x0.

The owner or the controller of the contracts can change the address of privileged addressed using functions such as setController and setBLabs:

https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol#L205-L212 https://github.com/balancer-labs/balancer-core/blob/master/contracts/BFactory.sol#L51-L54

However, these functions do not check for invalid values (e.g. 0x0) and they work in a single transaction.

Exploit Scenario

Alice creates a pool. She uses some off-chain code to manage it . However, a software issue in her code calls the setController function with an uninitialized value (0x0). The BPool code accepts this new value and the locks up Alice's pool. As a result of that, she will need to create a new pool.

Recommendation

Short term, split this important functionality in several functions. For instance, to change the current controller, implement setController and acceptController which reject any null address. Additionally, add a renounceController function that allows to set the controller to 0x0 if needed.

Long term, use Echidna and Manticore to verify that the administrative addresses cannot be set to incorrect values.

[mikemcdonald]

Won't fix. setBLabs won't be used in bronze since the EXIT_FEE is 0. And the additional UX complexity does not outweigh the benefits of splitting setController