Frax Ether Liquid Staking contest - obront's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L69 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L85

Vulnerability details

Impact

The sfrxETH.sol contract has depositWithSignature() and mintWithSignature() functions, with the goal of allowing one user to submit a transaction on behalf of another user with a properly signed message.

However, as it's implemented, these two functions require the owner to submit the transaction themselves, making them useless and removing the functionality from the protocol.

Proof of Concept

In order to create the approval for the deposit, depositWithSignature() calls asset.permit(msg.sender, address(this), amount, deadline, v, r, s);

In the OpenZeppelin ERC20Permit contract used, permit() is as follows:

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual override {
        require(block.timestamp <= deadline, "ERC20Permit: expired deadline");

        bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

        bytes32 hash = _hashTypedDataV4(structHash);

        address signer = ECDSA.recover(hash, v, r, s);
        require(signer == owner, "ERC20Permit: invalid signature");

        _approve(owner, spender, value);
    }

As you can see, the first argument is the owner, which must match the signer of the message. If this is the case, the contract is confident that the owner did in fact approve the spending.

However, in this case, msg.sender is passed to the function. This means the function will only approve the transfer if the owner themselves sends the message, which defeats the purpose of having a signature (since the transaction itself is signed by the owner).

Checking the test suite to see how this slipped past, you can look to frxETH_sfrxETH_combo.t.sol:test_DepositWithSignatureLimitedPermit(). In it, we call vm.prank(owner) before testing the function, which allows the function to work as expected. If any other user called the function with owner's signature, the function would fail.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Add owner as an argument to depositWithSignature() and mintWithSignature() and use that address as the first argument to asset.permit().