Article realeased on Oct 21, 2022
Introduction
Wyvern v2.2 is a set of smart contracts that are used for asset exchange.
It was used by OpenSea for a long time, even though they moved on to Seaport few months ago.
In this article, we discuss the three known vulnerabilities of Wyvern v2.2 and how to patch them.
•
DoS via Uninitialized Implementation, found in October 2019
•
Order Hashing Vulnerability, found in Q1 2022
•
ArrayUtils Memory Corruption, found in September 2022
1. DoS via Uninitialized Implementation
The first one is a classic - a vulnerability due to not initializing the implementation contract.
In Wyvern, each user has its own OwnableDelegateProxy - a proxy contract that has delegateProxyImplementation, an instance of AuthenticatedProxy as implementation.
It is used to call contracts (for example, to NFT addresses for NFT transfer) at the end of the asset exchange. In a sense, Wyvern allows users to pay digital assets in exchange of a function call via seller’s proxy contract. We see this in the codes below.
// WyvernProxyRegistry.sol
constructor () public {
delegateProxyImplementation = new AuthenticatedProxy();
}
Solidity
// ProxyRegistry.sol
function registerProxy() public returns (OwnableDelegateProxy proxy) {
require(proxies[msg.sender] == address(0));
proxy = new OwnableDelegateProxy(msg.sender, delegateProxyImplementation, abi.encodeWithSignature("initialize(address,address)", msg.sender, address(this)));
proxies[msg.sender] = proxy;
return proxy;
}
Solidity
// ExchangeCore.sol
/* Retrieve delegateProxy contract. */
OwnableDelegateProxy delegateProxy = registry.proxies(sell.maker);
/* Proxy must exist. */
require(delegateProxy != address(0));
/* Assert implementation. */
require(delegateProxy.implementation() == registry.delegateProxyImplementation());
/* Access the passthrough AuthenticatedProxy. */
AuthenticatedProxy proxy = AuthenticatedProxy(delegateProxy);
// some code here
/* Execute funds transfer and pay fees. */
uint price = executeFundsTransfer(buy, sell);
/* Execute specified call through proxy. */
require(proxy.proxy(sell.target, sell.howToCall, sell.calldata));
Solidity
If the delegateProxyImplementation is selfdestructed, everyone’s proxy contract would not be able to function correctly, leading to a denial of service.
However, if the delegateProxyImplementation is not initialized, one can simply initialize it to get ownership then delegatecall it to make it selfdestruct. We see this in the following code.
// AuthenticatedProxy.sol
function initialize (address addrUser, ProxyRegistry addrRegistry) public {
require(!initialized);
initialized = true;
user = addrUser;
registry = addrRegistry;
}
function proxy(address dest, HowToCall howToCall, bytes calldata) public returns (bool result) {
require(msg.sender == user || (!revoked && registry.contracts(msg.sender)));
if (howToCall == HowToCall.Call) {
result = dest.call(calldata);
} else if (howToCall == HowToCall.DelegateCall) {
result = dest.delegatecall(calldata);
}
return result;
}
Solidity
The patch to this is simple, you simply need to add initialization after creating delegateProxyImplementation. This patch was added in the Wyvern v3 contract.
// WyvernRegistry.sol
AuthenticatedProxy impl = new AuthenticatedProxy();
impl.initialize(address(this), this);
impl.setRevoke(true);
delegateProxyImplementation = address(impl);
Solidity
2. Order Hashing Vulnerability
One of the most important things when you utilize ECDSA signatures in your contract is to figure out which values your users should sign and how. In Wyvern v2.2, the users had to sign their Order structure which consists of many parameters that make up a buy/sell order.
// ExchangeCore.sol
/* An order on the exchange. */
struct Order {
/* Exchange address, intended as a versioning mechanism. */
address exchange;
/* Order maker address. */
address maker;
/* Order taker address, if specified. */
address taker;
/* Maker relayer fee of the order, unused for taker order. */
uint makerRelayerFee;
/* Taker relayer fee of the order, or maximum taker fee for a taker order. */
uint takerRelayerFee;
/* Maker protocol fee of the order, unused for taker order. */
uint makerProtocolFee;
/* Taker protocol fee of the order, or maximum taker fee for a taker order. */
uint takerProtocolFee;
/* Order fee recipient or zero address for taker order. */
address feeRecipient;
/* Fee method (protocol token or split fee). */
FeeMethod feeMethod;
/* Side (buy/sell). */
SaleKindInterface.Side side;
/* Kind of sale. */
SaleKindInterface.SaleKind saleKind;
/* Target. */
address target;
/* HowToCall. */
AuthenticatedProxy.HowToCall howToCall;
/* Calldata. */
bytes calldata;
/* Calldata replacement pattern, or an empty byte array for no replacement. */
bytes replacementPattern;
/* Static call target, zero-address for no static call. */
address staticTarget;
/* Static call extra data. */
bytes staticExtradata;
/* Token used to pay for the order, or the zero-address as a sentinel value for Ether. */
address paymentToken;
/* Base price of the order (in paymentTokens). */
uint basePrice;
/* Auction extra parameter - minimum bid increment for English auctions, starting/ending price difference. */
uint extra;
/* Listing timestamp. */
uint listingTime;
/* Expiration timestamp - 0 for no expiry. */
uint expirationTime;
/* Order salt, used to prevent duplicate hashes. */
uint salt;
}
Solidity
To sign this massive structure, the function hashOrder() is used.
function hashOrder(Order memory order) internal pure returns (bytes32 hash)
{
/* Unfortunately abi.encodePacked doesn't work here, stack size constraints. */
uint size = sizeOf(order);
bytes memory array = new bytes(size);
uint index;
assembly {
index := add(array, 0x20)
}
index = ArrayUtils.unsafeWriteAddress(index, order.exchange);
index = ArrayUtils.unsafeWriteAddress(index, order.maker);
index = ArrayUtils.unsafeWriteAddress(index, order.taker);
index = ArrayUtils.unsafeWriteUint(index, order.makerRelayerFee);
index = ArrayUtils.unsafeWriteUint(index, order.takerRelayerFee);
index = ArrayUtils.unsafeWriteUint(index, order.makerProtocolFee);
index = ArrayUtils.unsafeWriteUint(index, order.takerProtocolFee);
index = ArrayUtils.unsafeWriteAddress(index, order.feeRecipient);
index = ArrayUtils.unsafeWriteUint8(index, uint8(order.feeMethod));
index = ArrayUtils.unsafeWriteUint8(index, uint8(order.side));
index = ArrayUtils.unsafeWriteUint8(index, uint8(order.saleKind));
index = ArrayUtils.unsafeWriteAddress(index, order.target);
index = ArrayUtils.unsafeWriteUint8(index, uint8(order.howToCall));
index = ArrayUtils.unsafeWriteBytes(index, order.calldata);
index = ArrayUtils.unsafeWriteBytes(index, order.replacementPattern);
index = ArrayUtils.unsafeWriteAddress(index, order.staticTarget);
index = ArrayUtils.unsafeWriteBytes(index, order.staticExtradata);
index = ArrayUtils.unsafeWriteAddress(index, order.paymentToken);
index = ArrayUtils.unsafeWriteUint(index, order.basePrice);
index = ArrayUtils.unsafeWriteUint(index, order.extra);
index = ArrayUtils.unsafeWriteUint(index, order.listingTime);
index = ArrayUtils.unsafeWriteUint(index, order.expirationTime);
index = ArrayUtils.unsafeWriteUint(index, order.salt);
assembly {
hash := keccak256(add(array, 0x20), size)
}
return hash;
}
Solidity
here, ArrayUtils is used to layout the entire order in the memory.
We immediately see that there are some issues here - the calldata, replacementPattern, staticExtradata are bytes, i.e. variable length, yet they are directly concatenated and hashed. This may cause a collision as explained in https://swcregistry.io/docs/SWC-133.
The place to look deeper here is definitely
index = ArrayUtils.unsafeWriteBytes(index, order.calldata);
index = ArrayUtils.unsafeWriteBytes(index, order.replacementPattern);
index = ArrayUtils.unsafeWriteAddress(index, order.staticTarget);
index = ArrayUtils.unsafeWriteBytes(index, order.staticExtradata);
Solidity
which allows us to shift bytes between calldata, replacementPattern, staticTarget, staticExtradata. Later in the code, there will be additional requirement that the calldata and the replacementPattern has equal length. It’s also clear that the staticTarget should also be 20 bytes as it is an address. To exploit this malleability, we need to understand what calldata, replacementPattern means. Let’s take a deeper look inside Wyvern v2.2.
Wyvern v2.2 basically allows the buyer to transfer some ERC20/ETH so that the proxy contract of the seller calls (or delegatecalls) a target contract with a given calldata.
For this, the buyer and the seller needs to agree on the same calldata. However, it may be the case that one of them might not know the entire calldata, only a part of it.
For example, assume that an address wants to sell an NFT. In this case, the proxy contract of the seller will call NFT.transferFrom(from, to, id). In this case, the parameters from, id are already known, but to is not - it will be equal to anyone who matches the sell order.
This is where the replacementPattern comes in - it allows to “fill in” the parts that are currently unknown by the order creator. You can understand this parameter as the bitmask of the bits that can be changed by the taker of the order. Here, the to part of the replacementPattern will be filled with 1s, allowing the taker to fill that part with their own address.
The attack method is simple, brilliant and lethal. The goal is to modify the function selector.
It turns out that the usual function call is the ERC721’s transferFrom which has the 4byte selector of 0x23b872dd. It turns out that getApproved(uint) function, which is a view function, has the 4byte selector of 0x081812fc, which is only different in 10 bits.
Consider the following parameters for order, which is a maker buy order for an NFT.
calldata: 0x23b872dd[32 bytes of zero][32 bytes of "to" (maker's address)][32 bytes of NFT ID]
replacementPattern: 0x00000000[32 bytes of 1][32 bytes of zero][32 bytes of zero]
staticTarget: [20 bytes of zero]
staticExtradata: ""
Plain Text
We want to match this order without sending the NFT, stealing the money the maker put up.
calldata: 0x23b872dd[32 bytes of zero][part of maker's address]
replacementPattern: [part of maker's address][the remaining parts that makes the length of calldata and replacementPattern equal]
staticTarget: [20 bytes]
staticExtradata: [remaining parts]
Plain Text
Now if the maker’s address has 1’s in the bits where the two function selectors are different, the replacementPattern can be used to change the function call to getApproved(0).
There are around 16 ways to shift bytes to put maker’s address at the front of replacementPattern, so the probability of attack’s success is around 16/2^10 = 1/64.
This can be patched by applying EIP712 in the order hashing. The minimal patch is
•
add chainID in the hash for cross chain replay protection
•
keccak256 hash all the variable length parameters
although EIP712 is definitely the best way to patch in our opinion.
3. ArrayUtils Memory Corruption
As we mentioned before, Wyvern v2.2 has a calldata and replacementPattern to help the maker and the taker settle on a final contract calldata to use. To do this, the contract takes the buyer’s calldata, buyer’s replacementPattern, and seller’s calldata and does this.
•
If buyer’s replacementPattern’s bit is 0, take the buyer’s calldata
•
If buyer’s replacementPattern’s bit is 1, take the seller’s calldata
i.e. we let the replacementPattern select which calldata to actually use.
Similarly, the contract takes the seller’s calldata, seller’s replacementPattern and the buyer’s calldata and does the same thing. Then, the contract checks if the two final calldatas match.
/* Must match calldata after replacement, if specified. */
if (buy.replacementPattern.length > 0) {
ArrayUtils.guardedArrayReplace(buy.calldata, sell.calldata, buy.replacementPattern);
}
if (sell.replacementPattern.length > 0) {
ArrayUtils.guardedArrayReplace(sell.calldata, buy.calldata, sell.replacementPattern);
}
require(ArrayUtils.arrayEq(buy.calldata, sell.calldata));
Solidity
Here, the guardedArrayReplace function is the function that handles the calldatas and the replacementPatterns as stated above. It heavily uses assembly, as shown below.
// ArrayUtils.sol
function guardedArrayReplace(bytes memory array, bytes memory desired, bytes memory mask) internal pure {
require(array.length == desired.length);
require(array.length == mask.length);
uint words = array.length / 0x20;
uint index = words * 0x20;
assert(index / 0x20 == words);
uint i;
for (i = 0; i < words; i++) {
/* Conceptually: array[i] = (!mask[i] && array[i]) || (mask[i] && desired[i]), bitwise in word chunks. */
assembly {
let commonIndex := mul(0x20, add(1, i))
let maskValue := mload(add(mask, commonIndex))
mstore(add(array, commonIndex), or(and(not(maskValue), mload(add(array, commonIndex))), and(maskValue, mload(add(desired, commonIndex)))))
}
}
/* Deal with the last section of the byte array. */
if (words > 0) {
/* This overlaps with bytes already set but is still more efficient than iterating through each of the remaining bytes individually. */
i = words;
assembly {
let commonIndex := mul(0x20, add(1, i))
let maskValue := mload(add(mask, commonIndex))
mstore(add(array, commonIndex), or(and(not(maskValue), mload(add(array, commonIndex))), and(maskValue, mload(add(desired, commonIndex)))))
}
} else {
/* If the byte array is shorter than a word, we must unfortunately do the whole thing bytewise.
(bounds checks could still probably be optimized away in assembly, but this is a rare case) */
for (i = index; i < array.length; i++) {
array[i] = ((mask[i] ^ 0xff) & array[i]) | (mask[i] & desired[i]);
}
}
}
Solidity
We see that array, desired, mask is given - and the mask is the bitmask of where the desired bytes can be used. In the end, our goal is to have (~mask & array) | (mask & desired) in the place of array. We’ll decompose this contract and see how it works.
require(array.length == desired.length);
require(array.length == mask.length);
uint words = array.length / 0x20;
uint index = words * 0x20;
assert(index / 0x20 == words);
Solidity
The contract checks that array, desired, mask has the same length.
We will denote its length as len, and write len = 32 * words + rem where 0 <= rem < 32.
bytes memory layout
000000000000000000000000000000000000000000000000000000000000006a
[ 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
words | 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
] 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
0123456789abcdef012300000000000000000000000000000000000000000000
[__________________][__________________________________________]
rem usually zero bytes
len = 32 * words + rem
Plain Text
// ArrayUtils.sol
for (i = 0; i < words; i++) {
/* Conceptually: array[i] = (!mask[i] && array[i]) || (mask[i] && desired[i]), bitwise in word chunks. */
assembly {
let commonIndex := mul(0x20, add(1, i))
let maskValue := mload(add(mask, commonIndex))
mstore(add(array, commonIndex), or(and(not(maskValue), mload(add(array, commonIndex))), and(maskValue, mload(add(desired, commonIndex)))))
}
}
Solidity
Here, the first 32 * words bytes (i.e. in range [0, 32 * words)) are handled in a for loop.
The mload operations are used to get array values, then bit operations are used to get the final value (~mask & array) | (mask & desired), which is then mstore'ed into the array.
Then, we deal with the final rem bytes.
/* Deal with the last section of the byte array. */
if (words > 0) {
/* This overlaps with bytes already set but is still more efficient than iterating through each of the remaining bytes individually. */
i = words;
assembly {
let commonIndex := mul(0x20, add(1, i))
let maskValue := mload(add(mask, commonIndex))
mstore(add(array, commonIndex), or(and(not(maskValue), mload(add(array, commonIndex))), and(maskValue, mload(add(desired, commonIndex)))))
}
} else {
/* If the byte array is shorter than a word, we must unfortunately do the whole thing bytewise.
(bounds checks could still probably be optimized away in assembly, but this is a rare case) */
for (i = index; i < array.length; i++) {
array[i] = ((mask[i] ^ 0xff) & array[i]) | (mask[i] & desired[i]);
}
}
Solidity
There is a if branch, which is on whether words > 0.
If words > 0, the bytes at position [32 * words, 32 * words + 32) are handled as before.
If words = 0, then a for loop handles bytes from [32 * words, 32 * words + rem) byte by byte. In other words, each loop handles a single byte. As it is mentioned in the comments, this is unfortunate as this is less gas-efficient than doing 32 bytes at once.
The vulnerability here is that when rem = 0, words > 0 the memory at position [32 * words, 32 * words + 32) may belong to another variable, yet the function overwrites it with another value. This may lead to memory corruption, which could, in an unfortunate case, lead to storage corruption as well. The medium article by BlockSec provides one such example with Foundry.
BlockSec notes that the vulnerability might be caused from mistakenly thinking that words > 0 was the correct branch condition, although the correct one is rem > 0. This is definitely a good idea, and changing the condition to rem > 0 works if our memory layout guarantees that there are no non-zero bytes in the range [32 * words + rem, 32 * words + 32) which will be also overwritten despite technically being out of range of the bytes variable we are working with.
Here’s our different take on the vulnerability. A deeper look in the code suggests two things.
•
The code actually wants us to handle bytes exactly in range [32 * words, 32 * words + rem), and not just overwrite the entire word in [32 * words, 32 * words + 32).
◦
If not, there is no reason for the for loop in the second case to handle one byte in each loop. It would’ve just handled 32 bytes all at once, as before. However, the contract opts to handle byte-by-byte, even though the writers are aware that it is less gas-efficient.
◦
It also makes a lot of sense - this contract is full of inline assembly, and it should be extra careful when dealing with the memory. Making additional assumptions (on what bytes are at [32 * words + rem, 32 * words + 32)) about the layout seems dangerous.
•
The comment /* This overlaps with bytes already set but is still more efficient than iterating through each of the remaining bytes individually. */ currently does not make sense. The bytes already set are in range [0, 32 * words) and the bytes that are being set now are in range [32 * words, 32 * words + 32), which doesn’t overlap at all.
Looking at the two facts mentioned above, we believe that we found the true mistake.
Our conclusion is
•
The branch condition words > 0 is correct.
•
The commonIndex in the case words > 0 should be array.length.
How does this work? First, let’s see what happens when the patch above is added.
If words > 0, the commonIndex is equal to 32 * words + rem. Therefore, after taking the first 32 bytes for the bytes length into consideration, the bytes we handle is in range of [32 * (words - 1) + rem, 32 * words + rem). This solves all the vulnerabilities and all the mystery we have.
•
Now, the contract covers exactly the bytes in range [0, 32 * words + rem) as desired.
•
The bytes already set are in range [0, 32 * words) and the bytes being set now is in range of [32 * (words - 1) + rem, 32 * words + rem) which overlaps at [32 * (words - 1) + rem, 32 * words), so the comment now matches the code as well.
•
This tactic only works when 32 * (words - 1) + rem >= 0, which requires words > 0.
bytes memory layout
000000000000000000000000000000000000000000000000000000000000006a
[ 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
words | 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
] 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
0123456789abcdef012300000000000000000000000000000000000000000000
[__________________][__________________________________________]
rem usually zero bytes
red: covered by the initial for loop, in range [0, 32 * words)
purple: the overlap of the initial for loop and the final part
in range [32 * (words - 1) + rem, 32 * words + rem)
blue: the bytes that only the final part covered,
in range [32 * words, 32 * words + rem)
Plain Text
Therefore, we believe that this is the ultimate fix for the ArrayUtils vulnerability.
We also used some foundry fuzzing to check that the memory in [32 * words + rem, 32 * words + 32) remain untouched, but this is a story for another blog post. Thanks for reading!
About KALOS
KALOS is a flagship service of HAECHI LABS, the leader of the global blockchain industry. We bring together the best Web2 and Web3 experts. Security Researchers with expertise in cryptography, leaders of the global best hacker team, and blockchain/smart contract experts are responsible for securing your Web3 service.
We have secured over $60b worth of crypto assets across 400+ global crypto projects — L1/L2 projects, defi protocols, P2E games, and bridges — notably 1inch, SushiSwap, Badger DAO, SuperRare, Klaytn and Chainsafe.
KALOS is the only blockchain technology company selected for the Samsung Electronics Startup Incubation Program in recognition of our expertise. We have also received technology grants from the Ethereum Foundation and Ethereum Community Fund.
Secure your smart contracts with KALOS.
•
Email: audit@kalos.xyz
•
Official website: https://kalos.xyz
•
Twitter: https://twitter.com/kalos_security
