Lines of code
<https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L448&gt;
<https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L608&gt;
<https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L621&gt;
<https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L634&gt;
<https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L647&gt;

Vulnerability details

Impact

It is not possible to pause/unpause vaults and adaptors nor add staking reward tokens since the _verifyCreatorOrOwner() function reverts due to a logical error.

Proof of Concept

The following logic is used to determine if msg.sender is a creator or owner of the contract:

    if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

But this reverts if either msg.sender == owner or msg.sender == metadata.creator while owner != metadata.creator. Core functionality that is very important is therefore unavailable.

Most importantly, pausing/unpausing vaults and adaptors is impossible. These are functions needed for basic safety and the protocol is at risk if pausing is not possible. Pausing adapters for example are used to quickly exit the underlying protocol to secure users funds if necessary, this will currently not be possible.

Tests has been written to test this functionality but in the tests owner == creator so it does not revert as it would in a live contract. In VaultController.t.sol address(this) is set as the owner of the deployed controller, see L188.

When test are running that call _verifyCreatorOrOwner() vaults are always deployed with address(this) as the creator. With the following change to the test test__pauseAdapters()

function test__pauseAdapters() public {

	address[] memory targets = new address[](1);

	addTemplate("Adapter", templateId, adapterImpl, true, true);

	addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);

	addTemplate("Vault", "V1", vaultImpl, true, true);

	vm.startPrank(bob); // ADD this

	address vault = deployVault();

	vm.stopPrank(); // ADD this

	targets[0] = vault;

	controller.pauseAdapters(targets);

	assertTrue(IPausable(IVault(vault).adapter()).paused());

}

We can deploy a vault where “Bob” is the creator and as expected this test now fails.

Tools Used

manual review

Recommended Mitigation Steps

Change the code to

if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender); 

The function will now only revert if msg.sender is neither owner nor creator.

---

The text was updated successfully, but these errors were encountered:

All reactions