Security Vulnerabilities and Access Control Issues in L1ERC20Bridge Smart Contract

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol/README.md?plain=1#L63

Vulnerability details

Impact

Detailed description of the impact of this finding.

Access Control and Permissions:

Error: The tranferTokenToSharedBridge function lacks proper access control, allowing any caller to transfer tokens to the shared bridge.

Solution: Implement access control checks to restrict the tranferTokenToSharedBridge function to specific roles or contracts. You can use access control modifiers or require statements to ensure that only authorized users can call this function. For example:

function tranferTokenToSharedBridge(address _token, uint256 _amount) external { require(msg.sender == address(sharedBridge), "Not authorized"); // Transfer tokens to the shared bridge }

Data Availability Issues:

Error: The mappings depositAmount and isWithdrawalFinalized are used to store data related to deposits and withdrawals, but there's no access control or validation mechanism to ensure data integrity or restrict access.

Solution: Implement access control checks and validation mechanisms to ensure that only authorized users can access or modify the data stored in these mappings. You can use modifiers or require statements to enforce access control and validate inputs before updating the mappings.

EVM Compatibility Attacks:

Error: The contract lacks protection against reentrancy attacks, especially in functions like deposit and claimFailedDeposit where external calls are made and state is modified.

Solution: Use the nonReentrant modifier to prevent reentrancy attacks in functions that involve external calls or state modifications. You can apply this modifier to relevant functions to ensure that reentrant calls cannot occur. Here's an example of how to implement the nonReentrant modifier:

modifier nonReentrant() { require(!_locked, "Reentrant call detected"); locked = true; ; _locked = false; }

Ensure that the _locked variable is properly initialized and reset within the modifier. Apply this modifier to relevant functions to protect against reentrancy attacks.

Gas-related Vulnerabilities:

Error: Gas limits for external calls are passed as parameters to functions like deposit, which may lead to potential gas-related vulnerabilities if not set appropriately.

Solution: Review the gas limits passed to external calls in functions like deposit and ensure that they are set appropriately based on the expected gas consumption. Consider using safe gas limits and performing gas estimation to prevent potential gas-related exploits. Additionally, consider optimizing gas usage in critical functions to minimize the risk of gas-related vulnerabilities.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tool Used

Remix

Recommended Mitigation Steps

Assessed type

Error

[c4-judge]

alex-ppg marked the issue as duplicate of #28

[c4-sponsor]

razzorsec (sponsor) disputed

[razzorsec]

Incorrect details were provided, hence we considered this as invalid. For instance we do have access control for the concerned function above require(msg.sender == address(sharedBridge), "Not authorized");

[razzorsec]

We agreed to mark it as invalid again for the same reason as #28

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid