BadgerDAO ibBTC Wrapper contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Malicious user can monitor SetPricePerShare event and, if it was run long enough time ago and market moved, but, since there were no SetPricePerShare fired, the contract's pricePerShare is outdated, so a user can mint() with pricePerShare that is current for contract, but outdated for market, then wait for price update and burn() with updated pricePerShare, yielding risk-free profit at expense of contract holdings.

Proof of Concept

WrappedIbbtcEth updates pricePerShare variable by externally run updatePricePerShare function. The variable is then used in mint/burn/transfer functions without any additional checks, even if outdated/stalled. This can happen if the external function wasn't run for any reason. The variable is used via balanceToShares function: https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtcEth.sol#L155

This is feasible as updatePricePerShare to be run by off-chain script being a part of the system, and malfunction of this script leads to contract exposure by stalling the price. The malfunction can happen both by internal reasons (bugs) and by external ones (any system-level dependencies, network outrages). updatePricePerShare function: https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtcEth.sol#L72

Recommended Mitigation Steps

The risk comes with system design. Wrapping price updates with contract level variable for gas costs minimization is a viable approach, but it needs to be paired with corner cases handling. One of the ways to reduce the risk is as follows:

Introduce a threshold variable for maximum time elapsed since last pricePerShare update to WrappedIbbtcEth contract.

Then 2 variants of transferFrom and transfer functions can be introduced, both check condition {now - time since last price update < threshold}. If condition holds both variants do the transfer. If it doesn't the first variant reverts, while the second do costly price update. I.e. it will be cheap transfer, that works only if price is recent, and full transfer, that is similar to the first when price is recent, but do price update on its own when price is stalled. This way this full transfer is guaranteed to run and is usually cheap, costing more if price is stalled and it does the update.

After this whenever scheduled price update malfunctions (for example because of network conditions), the risk will be limited by market volatility during threshold time at maximum, i.e. capped.

Example code:

	// Added threshold

uint256 public pricePerShare;
uint256 public lastPricePerShareUpdate;
	uint256 public priceUpdateThreshold;
	
	event SetPriceUpdateThreshold(uint256 priceUpdateThreshold);
	
/// ===== Permissioned: Price update threshold =====
function setPriceUpdateThreshold(uint256 _priceUpdateThreshold) external onlyGovernance {
    priceUpdateThreshold = _priceUpdateThreshold;
    emit SetPriceUpdateThreshold(priceUpdateThreshold);
}
	
	// The only difference with current transfer code is that Full versions call balanceToSharesFull

function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
    uint256 amountInShares = balanceToShares(amount);

    _transfer(sender, recipient, amountInShares);
    _approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amountInShares, "ERC20: transfer amount exceeds allowance"));
    return true;
}

function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
    uint256 amountInShares = balanceToShares(amount);

    _transfer(_msgSender(), recipient, amountInShares);
    return true;
}
	
function transferFromFull(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
    uint256 amountInShares = balanceToSharesFull(amount);

    _transfer(sender, recipient, amountInShares);
    _approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amountInShares, "ERC20: transfer amount exceeds allowance"));
    return true;
}

function transferFull(address recipient, uint256 amount) public virtual override returns (bool) {
    uint256 amountInShares = balanceToSharesFull(amount);

    _transfer(_msgSender(), recipient, amountInShares);
    return true;
}
	
	// Now balanceToShares first checks if the price is stale
	// And reverts if it is
	// While balanceToSharesFull do the same check
	// But asks for price instead of reverting
	// Having guaranteed execution with increased costs sometimes
	// Which is fully deterministic, as user can track SetPricePerShare event
	// To understand whether it be usual or increased gas cost if the function be called now

function balanceToShares(uint256 balance) public view returns (uint256) {
			require(block.timestamp < lastPricePerShareUpdate + priceUpdateThreshold, "Price is stalled");
    return balance.mul(1e18).div(pricePerShare);
}
	
function balanceToSharesFull(uint256 balance) public view returns (uint256) {
			if (block.timestamp >= lastPricePerShareUpdate + priceUpdateThreshold) {
					updatePricePerShare();
			}
    return balance.mul(1e18).div(pricePerShare);
}

Handle

hyh

Vulnerability details

Impact

If price feed is manipulated in any way or there is any malfunction based volatility on the market, both contracts will pass it on a user. In the same time it's possible to construct mitigation mechanics for such cases, so user economics be affected by sustainable price movements only. As price outrages provide a substantial attack surface for the project it's worth adding some complexity to the implementation.

Proof of Concept

In WrappedIbbtcEth pricePerShare variable is updated by externally run updatePricePerShare function, https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtcEth.sol#L72, and then used in mint/burn/transfer functions without additional checks via balanceToShares function: https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtcEth.sol#L155.

In WrappedIbbtc price is requested via pricePerShare function, https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtc.sol#L123, and used in the same way without additional checks via balanceToShares function, https://github.com/code-423n4/2021-10-badgerdao/blob/main/contracts/WrappedIbbtc.sol#L147.

Recommended Mitigation Steps

Introduce minting/burning query that runs on schedule, separating user funds contribution and actual mint/burn. With user deposit or burn the corresponding action to be added to commitment query, which execution for mint or redeem will later be sparked by off-chain script according to fixed schedule. This also can be open to public execution with gas compensation incentive, for example as it's done in Tracer protocol: https://github.com/tracer-protocol/perpetual-pools-contracts/blob/develop/contracts/implementation/PoolKeeper.sol#L131 Full code of an implementation is too big to include it in the report, but viable versions are available publicly (Tracer protocol version can be found at the same repo, https://github.com/tracer-protocol/perpetual-pools-contracts/blob/develop/contracts/implementation/PoolCommitter.sol).

Once the scheduled mint/redeem query is added, the additional logic to control for price outliers will become possible there, as in this case mint/redeem execution can be conditioned to happen on calm market only, where various definitions of calm can be implemented. One of the approaches is to keep track of recent prices and require that new price each time be within a threshold from median of their array.

Example:

// Introduce small price tracking arrays: uint256[] private times; uint256[] private prices;

// Current position in array uint8 curPos;

// Current length, grows from 0 to totalMaxPos as prices are being added uint8 curMaxPos;

// Maximum length, we track up to totalMaxPos prices uint8 totalMaxPos = 10;

// Price movement threshold uint256 moveThreshold = 0.1*1e18;

We omit the full implementation here as it is lengthy enough and can vary. The key steps are:

• Run query for scheduled mint/redeem with logic: if next price is greater than median of currently recorded prices by threshold, add it to the records, but do not mint/redeem.
• That is, when scheduled mint/redeem is run, on new price request, WrappedIbbtcEth.core.pricePerShare() or WrappedIbbtc.oracle.pricePerShare(), get newPrice and calculate current price array median, curMed
• prices[curPos] = newPrice
• if (curMaxPos < totalMaxPos) {curMaxPos += 1}
• if (curPos == curMaxPos) {curPos = 0} else {curPos += 1}
• if (absolute_value_of(newPrice - curMed) < moveThreshold * curMed / 1e18) {do_mint/redeem; return_0_status}
• else {return_1_status}

Schedule should be frequent enough, say once per 30 minutes, which is kept while returned status is 0. While threshold condition isn't met and returned status is 1, it runs once per 10 minutes. The parameters here are subject to calibration. This way if the price movement is sustained the mint/redeem happens after price array median comes to a new equilibrium. If price reverts, the outbreak will not have material effect mint/burn operations. This way the contract vulnerability is considerably reduced as attacker would need to keep distorted price for period long enough, which will happen after the first part of deposit/withdraw cycle. I.e. deposit and mint, burn and redeem operations will happen not simultaneously, preventing flash loans to be used to elevate the quantities, and for price to be effectively distorted it would be needed to keep it so for substantial amount of time.