c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L242 https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L242

Vulnerability details

Proof of Concept

requestDepositWithPermit requests asset deposit for a receiver to be included in the next epoch execution with a permit option. requestRedeemWithPermit request share redemption for a receiver to be included in the next epoch execution with a permit option.

File: LiquidityPool.sol

    /// @notice Similar to requestDeposit, but with a permit option.
    function requestDepositWithPermit(uint256 assets, address owner, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
        public
    {
        ERC20PermitLike(asset).permit(owner, address(investmentManager), assets, deadline, v, r, s);
        investmentManager.requestDeposit(assets, owner);
        emit DepositRequested(owner, assets);
    }

     ....................................................

      /// @notice Similar to requestRedeem, but with a permit option.
    function requestRedeemWithPermit(uint256 shares, address owner, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
        public
    {
        share.permit(owner, address(investmentManager), shares, deadline, v, r, s);
        investmentManager.requestRedeem(shares, owner);
        emit RedeemRequested(owner, shares);
    }

Both the above functions (requestDepositWithPermit & requestRedeemWithPermit) call the permit function with the correct parameters.

The permit function updates the nonce & calculates the digest with the necessary parameters & calls the _isValidSignature to verify the permit.

File: ERC20.sol

       function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
        external
    {
        permit(owner, spender, value, deadline, abi.encodePacked(r, s, v));
    }

233     require(_isValidSignature(owner, digest, signature), "ERC20/invalid-permit");

The issue lies in the first part of the _isValidSignature. The signature length is matched & the corresponding values of v, r & s are loaded from the memory & verified against the signer/owner address using ecrecover.

File: ERC20.sol

       // --- Approve by signature ---
    function _isValidSignature(address signer, bytes32 digest, bytes memory signature) internal view returns (bool) {
        if (signature.length == 65) {
            bytes32 r;
            bytes32 s;
            uint8 v;
            assembly {
                r := mload(add(signature, 0x20))
                s := mload(add(signature, 0x40))
                v := byte(0, mload(add(signature, 0x60)))
            }
            if (signer == ecrecover(digest, v, r, s)) {
                return true;
            }
        }

But the problem arises in the order inside the ecrecover() as the order of v, r & s are different that is supplied in permit() which is passed as signature in the _isValidSignature.

Inside the permit function the order of v, r & s is

242     abi.encodePacked(r, s, v)

& while verifying in _isValidSignature function, the order inside ecrecover() is

206     ecrecover(digest, v, r, s)

Impact

Thus everytime the user call the request or redeem function with a permit, the first condition will always return false despite the owner having provided the correct credentials because the address of the signer will never match the recovered address. In a situation where if the owner does not properly implement the IERC1271 interface & having provided the correct parameters, the owner might not be able to deposit or redeem even after holding a valid permit.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the correct order inside ecrecover()

- 206        if (signer == ecrecover(digest, v, r, s)) {
+ 206       if (signer == ecrecover(digest, r, s, v)) {

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #29

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid

Shubh0412 commented 1 year ago

This issue is not a duplicate of #29.

The order of v, r, s in line 242 & inside the signature check on line 206 will always return false.

As a proof, I have created this contract in REMIX with the same order of v, r, s as in the centrifuge contract. For reference, this Youtube tutorial has been followed to verify the signature. Adjustment to the code in the tutorial has been made with the addition of message_vrs() to demonstrate the exact bug.

Not every parameter has been checked as their presence will not change the outcome because the submission/issue focuses on the order of variables v, r, s used inside the contract.

You can use the exact values as mentioned in the comments inside the contract although the signature will be different because of the metamask account used while signing.


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

/* Signature Verification

How to Sign and Verify
# Signing
1. Create message to sign
2. Hash the message
3. Sign the hash (off chain, keep your private key secret)

# Verify
1. Recreate hash from the original message
2. Recover signer from signature and hash
3. Compare recovered signer to claimed signer
*/

contract VerifySignature {
    /* 1. Unlock MetaMask account
    ethereum.enable()
    */

    /* 2. Get the encoded message value using v, r & s
    message_vrs(
        27,
        0x636f666665650000000000000000000000000000000000000000000000000000,
        0x646f6e7574730000000000000000000000000000000000000000000000000000
        )

        encodedValue = "0x636f666665650000000000000000000000000000000000000000000000000000646f6e75747300000000000000000000000000000000000000000000000000001b"
    */

    function message_vrs(
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public pure returns (bytes memory) {
        return abi.encodePacked(r, s, v);
    }

    /* 3. Get message hash to sign
    getMessageHash(
        0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2,
        1e18,
        "0x636f666665650000000000000000000000000000000000000000000000000000646f6e75747300000000000000000000000000000000000000000000000000001b",
        1
    )

    hash = "0xc9263ba83648f50e1bbd5fef82f1ee14745ac095c13c501e7e8cea4c8e6bb417"
    */
    function getMessageHash(
        address _to,
        uint _amount,
        bytes memory _message,
        uint _nonce
    ) public pure returns (bytes32) {
        return keccak256(abi.encodePacked(_to, _amount, _message, _nonce));
    }

    /* 4. Sign message hash
    # using browser
    account = "copy paste account of signer here"
    ethereum.request({ method: "personal_sign", params: [account, hash]}).then(console.log)

    # using web3
    web3.personal.sign(hash, web3.eth.defaultAccount, console.log)

    Signature will be different for different accounts
    0x1983d2791b4789d76bcb220d074860ed8a6ff0bb35a3df710c902d466796ae5458d2afa1d113d5c8c5caba97411459a3713a388bfedf3e82801707a26333d6531b
    */
    function getEthSignedMessageHash(
        bytes32 _messageHash
    ) public pure returns (bytes32) {
        /*
        Signature is produced by signing a keccak256 hash with the following format:
        "\x19Ethereum Signed Message\n" + len(msg) + msg
        */
        return
            keccak256(
                abi.encodePacked("\x19Ethereum Signed Message:\n32", _messageHash)
            );
    }

    /* 5. Verify signature
    signer = 0xB273216C05A8c0D4F0a4Dd0d7Bae1D2EfFE636dd
    to = 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
    amount = 1e18
    v = 27
    r = 0x636f666665650000000000000000000000000000000000000000000000000000
    s = 0x646f6e7574730000000000000000000000000000000000000000000000000000
    nonce = 1
    signature =
        0x1983d2791b4789d76bcb220d074860ed8a6ff0bb35a3df710c902d466796ae5458d2afa1d113d5c8c5caba97411459a3713a388bfedf3e82801707a26333d6531b
    */
    function verify(
        address _signer,
        address _to,
        uint _amount,
        uint8 v,
        bytes32 r,
        bytes32 s,
        uint _nonce,
        bytes memory signature
    ) public pure returns (bool) {
        bytes32 messageHash = getMessageHash(_to, _amount, abi.encodePacked(v, r, s), _nonce);
        bytes32 ethSignedMessageHash = getEthSignedMessageHash(messageHash);

        return recoverSigner(ethSignedMessageHash, signature) == _signer;
    }

    function recoverSigner(
        bytes32 _ethSignedMessageHash,
        bytes memory _signature
    ) public pure returns (address) {
        (bytes32 r, bytes32 s, uint8 v) = splitSignature(_signature);

        return ecrecover(_ethSignedMessageHash, v, r, s);
    }

    function splitSignature(
        bytes memory sig
    ) public pure returns (bytes32 r, bytes32 s, uint8 v) {
        require(sig.length == 65, "invalid signature length");

        assembly {
            /*
            First 32 bytes stores the length of the signature

            add(sig, 32) = pointer of sig + 32
            effectively, skips first 32 bytes of signature

            mload(p) loads next 32 bytes starting at the memory address p into memory
            */

            // first 32 bytes, after the length prefix
            r := mload(add(sig, 32))
            // second 32 bytes
            s := mload(add(sig, 64))
            // final byte (first byte of the next 32 bytes)
            v := byte(0, mload(add(sig, 96)))
        }

        // implicitly return (r, s, v)
    }

}

No matter how many times verify() is called , the result is always false.

If in message_vrs(), the variables inside abi.encodepacked are changed from (r, s, v) to (v, r, s) keeping all the other parameters constant, the verify() returns true.

The downside of this bug is explained in the Impact section.

Also there is a mistake in the recommended step. The order inside the permit() should be changed, not in ecrecover.

File: ERC20.sol

-               permit(owner, spender, value, deadline, abi.encodePacked(r, s, v));
+               permit(owner, spender, value, deadline, abi.encodePacked(v, r, s));

gzeon-c4 commented 1 year ago

Invalid, the r, s, v is unpacked correctly in this assembly block https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L202-L204