Contract fails to deliver promised returns, but doesn't lose value
Description
Brief/Intro
The setMigrationPermit() function before setting the migration flag checks if the migrator being used by caller is a valid migrator, if not, then the function reverts.
So if a user permits a migrator, and later the migrator is revoked from the migrator role by the admin (maybe because of malicious behaviour or account takeover), then the user won't be able to revoke the permit because the setMigrationPermit() function will require the provided address to be a migrator.
Vulnerability Details
The setMigrationPermit function in Staking.sol allows users to approve an address to migrate their positions. However, it enforces that the _migrator address must currently hold the MIGRATOR_ROLE.
functionsetMigrationPermit(address_migrator,bool_isMigrationPermitted)external{if(!hasRole(MIGRATOR_ROLE, _migrator))revertMigratorNotFound(_migrator);// @audit-issue Check prevents revocation if role is removed migrationPermits[_migrator][msg.sender]= _isMigrationPermitted;emitMigrationPermitUpdated(_migrator,msg.sender, _isMigrationPermitted);}
If a migrator is compromised or acts maliciously, the admin would revoke their MIGRATOR_ROLE. Once this happens, the check if (!hasRole(MIGRATOR_ROLE, _migrator)) will cause any call to setMigrationPermit to revert.
This prevents users who had previously approved this migrator from revoking their approval. Their approval remains pending true in the migrationPermits mapping forever, unless the role is granted back.
Impact Details
If the MIGRATOR_ROLE is ever re-granted to the removed address (e.g., accidental admin action, or operational oversight), the previously removed actor would immediately regain the ability to migrate user positions without obtaining new consent. Users are essentially denied the ability to "clean up" their approvals and proactively secure their positions against a specific address.
Recommendations
Modify setMigrationPermit to allow users to set _isMigrationPermitted to false even if the _migrator does not have the role. The check should only be enforced when granting permission (setting it to true).
function setMigrationPermit(address _migrator, bool _isMigrationPermitted) external {
// Only require the role if we are enabling the permit
if (_isMigrationPermitted && !hasRole(MIGRATOR_ROLE, _migrator)) revert MigratorNotFound(_migrator);
migrationPermits[_migrator][msg.sender] = _isMigrationPermitted;
emit MigrationPermitUpdated(_migrator, msg.sender, _isMigrationPermitted);
}
// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.23;
import {Test, console2} from "forge-std/Test.sol";
import {Staking} from "../src/Staking.sol";
import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import {IStakingV1} from "../src/interfaces/IStakingV1.sol";
contract Token is ERC20 {
constructor() ERC20("TestToken", "TTKN") {}
}
contract MigrationPermitIssueTest is Test {
Staking public staking;
Token public token;
address public admin = address(1);
address public manager = address(2);
address public pauser = address(3);
address public user = address(5);
address public maliciousMigrator = address(6);
function setUp() public {
token = new Token();
vm.startPrank(admin);
staking = new Staking(admin, manager, pauser, address(token));
vm.stopPrank();
}
function test_CannotRevokePermitAfterMigratorRemoved() public {
// 1. Admin grants migrator role to maliciousMigrator
vm.startPrank(admin);
staking.grantRole(staking.MIGRATOR_ROLE(), maliciousMigrator);
vm.stopPrank();
// 2. User permits the migrator
vm.startPrank(user);
staking.setMigrationPermit(maliciousMigrator, true);
vm.stopPrank();
// Check permit is true
assertTrue(staking.migrationPermits(maliciousMigrator, user));
// 3. Admin detects malicious behavior and revokes role
vm.startPrank(admin);
staking.revokeRole(staking.MIGRATOR_ROLE(), maliciousMigrator);
vm.stopPrank();
// 4. User tries to revoke permit to be safe
vm.startPrank(user);
// This should fail with the current implementation
vm.expectRevert(
abi.encodeWithSelector(IStakingV1.MigratorNotFound.selector, maliciousMigrator)
);
staking.setMigrationPermit(maliciousMigrator, false);
vm.stopPrank();
}
}
