Trapped approvals in Folks Finance Staking: a permit you could grant but never revoke

Folks Finance runs a staking system where users lock funds and, later, may need to have their position migrated, moved by a trusted address to a new contract, for example during an upgrade. That is powerful, so the design puts users in control: a migrator can only move your position if you have approved it, and you can withdraw that approval whenever you want. This finding is about that withdraw path quietly breaking, exactly when a user would most want to use it. It was submitted to the Immunefi audit competition for the contracts and is publicly disclosed.

The pieces, in plain terms

• A migrator is an address the protocol trusts to move staked positions. It holds a role called MIGRATOR_ROLE, which an admin can grant or revoke.
• A permit is a user's personal approval: setMigrationPermit(migrator, true) says "this migrator may move my position," and setMigrationPermit(migrator, false) takes it back.
• The rule the protocol promises is simple: a migrator cannot move your stake unless you approved it. Your permit is your seatbelt.

The function, and the check in the wrong place

Here is the approval function. One line decides everything:

function setMigrationPermit(address _migrator, bool _isMigrationPermitted) external {
    if (!hasRole(MIGRATOR_ROLE, _migrator)) revert MigratorNotFound(_migrator);

    migrationPermits[_migrator][msg.sender] = _isMigrationPermitted;
    emit MigrationPermitUpdated(_migrator, msg.sender, _isMigrationPermitted);
}

The check reads: if the target address does not currently hold MIGRATOR_ROLE, revert. That sounds reasonable, and for granting a permit it is: you should not be able to approve an address that is not a real migrator. The mistake is that this check runs on every call, without asking whether the user is granting or revoking. It guards the exit as tightly as the entrance.

Why a stuck "true" is dangerous

At first glance a dead migrator sounds harmless: if it has no role, it cannot migrate anything anyway. The danger is that the stale approval lives on in storage, waiting. Admins revoke a migrator's role precisely when they no longer trust it, a suspected key compromise being the obvious case. And roles come back:

• A governance mistake re-grants the role to the same address.
• A redeployment lands a new contract at the same address (CREATE2), inheriting old approvals.
• An admin key is compromised and the attacker simply re-grants the role.

The moment the role returns to that address, every user who could never revoke is exposed again, and this time the migrator can act on an approval the user has been trying to cancel. It can call migratePositionsFrom(user) and move their staked funds and rewards without their current consent. The user's seatbelt was cut at the one moment it mattered.

Walking the attack

1. Alice stakes and approves

   Alice stakes 10 ETH and calls setMigrationPermit(migrator, true). Her permit is on.

2. The admin pulls the role

   The migrator's key is suspected compromised, so the admin revokes MIGRATOR_ROLE from it.

3. Alice tries to protect herself, and can't

   She calls setMigrationPermit(migrator, false). It reverts with MigratorNotFound. Her approval is stuck as true.

4. The role comes back

   Later the role is re-granted to the same address. Alice's stale approval is still live.

5. Her funds move without consent

   The migrator calls migratePositionsFrom(alice) and takes her position.

Proof of concept

The test drops into StakingTest in test/Staking.t.sol. It stakes as Alice, approves the migrator, has the admin revoke the role, shows Alice's revoke call reverting, re-grants the role, and then migrates Alice's funds out from under her.

function test_Exploit_IrrevocableMigrationPermit() public {
    // Alice stakes 10 ether and grants the migrator a permit
    // ... stake setup ...
    vm.prank(alice);
    staking.setMigrationPermit(migrator, true);
    assertEq(staking.migrationPermits(migrator, alice), true);

    // Admin revokes MIGRATOR_ROLE (e.g. key suspected compromised)
    vm.prank(admin);
    staking.revokeRole(keccak256("MIGRATOR"), migrator);

    // Alice tries to revoke her permit -> REVERTS, she cannot protect herself
    vm.prank(alice);
    vm.expectRevert(abi.encodeWithSelector(IStakingV1.MigratorNotFound.selector, migrator));
    staking.setMigrationPermit(migrator, false);
    assertEq(staking.migrationPermits(migrator, alice), true);   // stuck as true

    // Later the role is re-granted to the same address
    vm.prank(admin);
    staking.grantRole(keccak256("MIGRATOR"), migrator);

    // The stale permit now lets the migrator take Alice's funds
    vm.prank(migrator);
    IStakingV1.UserStake[] memory migrated = staking.migratePositionsFrom(alice);
    assertEq(migrated[0].amount, 10 ether);
    assertEq(staking.getUserStakes(alice).length, 0);            // Alice has nothing left
}

[PASS] test_Exploit_IrrevocableMigrationPermit() (gas: 746664)
Suite result: ok. 1 passed; 0 failed; 0 skipped

The fix

A user must always be able to take back their own approval, whatever the migrator's current status. So the role check belongs only on the granting path, not the revoking one:

function setMigrationPermit(address _migrator, bool _isMigrationPermitted) external {
    // only require the role when GRANTING; revoking must always succeed
    if (_isMigrationPermitted && !hasRole(MIGRATOR_ROLE, _migrator))
        revert MigratorNotFound(_migrator);

    migrationPermits[_migrator][msg.sender] = _isMigrationPermitted;
    emit MigrationPermitUpdated(_migrator, msg.sender, _isMigrationPermitted);
}

Granting still validates that the migrator is real, so nothing gets weaker. Revoking now always goes through, so a user can switch off an old approval at any time, and there is no way to leave a live authorization stranded in storage.