zkSync Era - AkshaySrivastav's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L340-L350

Vulnerability details

Impact

The L1ERC20Bridge contract intends to impose a deposit limit on token bridging.

This deposit limit is stored in AllowList contract in tokenDeposit mapping.

    mapping(address => Deposit) public tokenDeposit;
    struct Deposit {
        bool depositLimitation;
        uint256 depositCap;
    }

The deposit limit can be changed by protocol owners using the Allowkist.setDepositLimit function.

Further, this deposit limit is enforced in the L1ERC20Bridge.claimFailedDeposit function.

    function claimFailedDeposit(
        ...
    ) external nonReentrant senderCanCallFunction(allowList) {
        ...
        // Change the total deposited amount by the user
        _verifyDepositLimit(_l1Token, _depositSender, amount, true);

        ...
    }
    
    function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) internal {
        IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(_l1Token);
        if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token

        if (_claiming) {
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
        } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

In the above _verifyDepositLimit function it can be seen that the totalDepositedAmountPerUser state is not updated when Deposit.depositLimitation is false.

This mechanism creates issue when deposit limits are toggled by the protocol owners.

Consider this scenario:

1. For a token TokenX there is no deposit cap, i.e. the Deposit.depositLimitation is false.
2. A user deposits some TokenX in the L1 bridge. Since there is no deposit limit on that token the L1ERC20Bridge.totalDepositedAmountPerUser doesn't get updated.
3. The L2 transaction for user deposit fails on ZkSync chain for any possible reason.
4. Now, the protocol owner decides to put a bridge deposit limit for TokenX. The Deposit.depositLimitation is set to true.
5. The user now tries to claim back his failed token deposit, but since the totalDepositedAmountPerUser wasn't updated originally for user, it reverts due to integer underflow error here.
6. User is not able to claim back his deposited tokens.

In this case, the user was not able to claim back his deposited token (whose L2 counterpart txn failed) resulting in loss of funds.

Proof of Concept

Provided above

Tools Used

Manual review

Recommended Mitigation Steps

Consider not enforcing deposit limit on claimFailedDeposit instead only enforce that on initial user deposits.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/governance/Governance.sol#L154-L158

Vulnerability details

Impact

The Governance contract has a securityCouncil state who has the right for calling the cancel function. This state is set using the updateSecurityCouncil function.

    modifier onlySelf() {
        require(msg.sender == address(this), "Only governance contract itself allowed to call this function");
        _;
    }
    modifier onlyOwnerOrSecurityCouncil() {
        require(
            msg.sender == owner() || msg.sender == securityCouncil,
            "Only the owner and security council are allowed to call this function"
        );
        _;
    }

    function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {
        emit ChangeSecurityCouncil(securityCouncil, _newSecurityCouncil);
        securityCouncil = _newSecurityCouncil;
    }
    
    function cancel(bytes32 _id) external onlyOwnerOrSecurityCouncil {
        require(isOperationPending(_id), "Operation must be pending");
        delete timestamps[_id];
        emit OperationCancelled(_id);
    }

The function is protected by the onlySelf modifier. This is done so that updating of security council happens in a time delayed way.

This leads to a scenario in which a rogue/malicious/compromised security council can prevent all future time delayed operations from executing, including the changing of security council itself.

Due to this, the rogue security council will keep on cancelling all operations that are scheduled by the protocol. Changing the security council will be impossible.

Proof of Concept

• Governance gets deployed and security council is set.
• The security council starts behaving maliciously (due to accounts getting compromised, team members getting rogue, etc).
• The security council starts cancelling all scheduled and pending operations. No more operations can be executed via Governance contract.
• Since changing the security council also needs to happen via Governance, the security council cannot be changed now.

Hence the Governance is in a complete DoS state. All funds held by Governance are stuck and all admin rights given to Governance are facing DoS.

Note: This issue is different than normal centralization risk issues as this describes a scenario where a rogue council can completely stop all operations of Governance including the council change operation as well.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the onlyOwner modifier for updateSecurityCouncil instead of onlySelf.

Assessed type

Context