code-423n4 / 2022-10-zksync-findings

3 stars 0 forks source link

1) THERE IS CONFLICT IN REQUIRE STATMENT. IN finalizeDeposit FUNCTION CHECKS require (msg.sender == l1Bridge, "mq"). IN bridgeMint FUNCTION IMPLEMENTATIONS onlyBridge Modifier CHECKS require(msg.sender == l2Bridge). #288

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/zksync/contracts/bridge/L2ERC20Bridge.sol#L58-L67 https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/zksync/contracts/bridge/L2StandardERC20.sol#L95

Vulnerability details

1)

Impact

Let assume if we calling bridgeMint() from finalizeDeposit function the msg.sender is l1bridge . if the msg.sender is not l1bridge can't run the finalizeDeposit

but as per bridgeMint() the msg.sender should be l2bridge. So it will cause the conflict . the bridgeMint() always fails since the caller function msg.sender is l1bridge.

modifier onlyBridge() { require(msg.sender == l2Bridge); _; }

function bridgeMint(address _to, uint256 _amount) external override onlyBridge { _mint(_to, _amount); emit BridgeMint(_to, _amount); }

PROOF OF CONCEPT

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/zksync/contracts/bridge/L2ERC20Bridge.sol#L58-L67

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/zksync/contracts/bridge/L2StandardERC20.sol#L95-L105

TOOLS : Manual Finding

Recommended Mitigation Steps

We need to change the require statement conditions which is suitable for both scenarios.

GalloDaSballo commented 1 year ago

Looks off

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor disputed

miladpiri commented 1 year ago

Caller of finalizeDeposit is l1Bridge, and caller of the strandardERC20Token is l2Bridge! So, totally invalid!

GalloDaSballo commented 1 year ago

Per confirmation by sponsor, incorrect finding

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid