Polygon Aave & DAI Bridge Integrations Security Review Report

The L1Escrow contract lacks automatic rebalancing logic in scenarios where the available balances of dai and sdai are insufficient to cover withdrawals triggered by the onMessageReceived function. This can result in asset management inefficiency.

When the onMessageReceived function is triggered and requires a withdrawal of funds, the contract should automatically perform rebalancing actions to ensure that the required funds are available for withdrawal, otherwise the non-rebalanced DAI can be used for the withdrawal (as the token depositing is done via try/catch construct there is possibility for DAI token to be present on the contract).

function onMessageReceived(
    address originAddress,
    uint32 originNetwork,
    bytes memory metadata
) external payable virtual {
    if (msg.sender != address(zkEvmBridge)) revert MessageInvalid();
    if (originAddress != destAddress) revert MessageInvalid();
    if (originNetwork != destId) revert MessageInvalid();
 
    (address recipient, uint256 amount) =
        abi.decode(metadata, (address, uint256));
 
    uint256 sdaiBalance = IERC20(address(sdai)).balanceOf(address(this));
    uint256 savingsBalance = sdai.previewRedeem(sdaiBalance);
    if (amount > savingsBalance) {
        sdai.withdraw(savingsBalance, recipient, address(this));
        dai.transfer(recipient, amount - savingsBalance);
    } else {
        sdai.withdraw(amount, recipient, address(this));
    }
 
    totalBridgedDAI -= amount;
    emit DAIClaimed(recipient, amount, totalBridgedDAI);
}

Recommend either incorporating rebalancing logic within the onMessageReceived function, or withdrawing (partially/fully) the non-rebalanced DAI.

The role ADMIN_ROLE is crucial for the L1Escrow.sol contract, in the event it accidentally renounced, revoked, or assigned to a wrong address, it becomes impossible to upgrade the contract.

Precaution measures should be taken to avoid such situations, including: enforcing 2-step process to transfer the role; not allowing to revoke/renounce the role, or allowing to revoke/renounce the role but with a delay and possibility to cancel the revocation/renouncement before the delay ends.

contract L1Escrow is Initializable, UUPSUpgradeable, AccessControlUpgradeable {
    ...
    function _authorizeUpgrade(address v) internal override onlyRole(ADMIN_ROLE) {}
    ...
}

use AccessControlDefaultAdminRulesUpgradeable instead of AccessControlUpgradeable

SafeERC20 library is used inconsistently. In some cases the safeApprove() and safeTransferFrom() functions are used with DAI token (L146, L153, L155 of L1Escrow.sol). In other cases plain transfer() function is used instead (L181, L248 of L1Escrow.sol) and its return value isn't checked.

contract L1Escrow is Initializable, UUPSUpgradeable, AccessControlUpgradeable {
    using SafeERC20 for IERC20;
 
    ...
 
    function bridgeToken(
        address recipient,
        uint256 amount,
        bool forceUpdateGlobalExitRoot
    ) external virtual {
        ...
>>      dai.safeTransferFrom(msg.sender, address(this), amount);
        ...
>>      dai.safeApprove(address(sdai), amount);
        try sdai.deposit(amount, address(this)) returns (uint256) {} catch {}
>>      dai.safeApprove(address(sdai), 0);
        ...
    }
 
    function onMessageReceived(
        address originAddress,
        uint32 originNetwork,
        bytes memory metadata
    ) external payable virtual {
        ...
        if (amount > savingsBalance) {
            sdai.withdraw(savingsBalance, recipient, address(this));
>>          dai.transfer(recipient, amount - savingsBalance);
        } else {
            sdai.withdraw(amount, recipient, address(this));
        }
        totalBridgedDAI -= amount;
        emit DAIClaimed(recipient, amount, totalBridgedDAI);
    }
}

Use safeTransfer() instead of transfer().

The purpose of the rebalance() function is to ensure the equilibrium of the total locked DAI within the L1Escrow contract.

Upon execution, the rebalance() function adds DAI tokens to the SDAI contract. Following this deposit, the rebalance() function redundantly invokes the sdai.approve() function with a value of 0. This redundant call is unnecessary, as the sdai.deposit() function already employs the transferFrom() mechanism, which inherently diminishes the allowance by the amount being transferred.

function rebalance() public {
    uint256 balance = IERC20(address(dai)).balanceOf(address(this));
    if (balance > totalProtocolDAI) {
        uint256 targetDepositAmount = balance - totalProtocolDAI;
        if (targetDepositAmount > 0.05 ether) {
            // Leave smol amount of DAI in this smart contract
            uint256 depositAmount = targetDepositAmount - 0.01 ether;
            dai.safeApprove(address(sdai), depositAmount);
            sdai.deposit(depositAmount, address(this));
            dai.safeApprove(address(sdai), 0);
            emit AssetRebalanced();
        }
    } else {
        uint256 sdaiBalance = IERC20(address(sdai)).balanceOf(address(this));
        uint256 savingsBalance = sdai.previewRedeem(sdaiBalance);
        uint256 withdrawAmount = totalProtocolDAI - balance;
        if (withdrawAmount > 0 && savingsBalance > withdrawAmount) {
            sdai.withdraw(withdrawAmount, address(this), address(this));
            emit AssetRebalanced();
        }
    }
}

Eliminate the redundant call to reduce the cost of executing the function.

The owner account is crucial for the L2Dai.sol contract, in the event it accidentally renounced, or transferred to a wrong address, it becomes impossible to upgrade the contract.

Precaution measures should be taken to avoid such situations, including: enforcing 2-step process to transfer the ownership; not allowing to renounce the ownership, or allowing to renounce it but with a delay and possibility to cancel the renouncement before the delay ends.

contract L2Dai is
    Initializable,
    UUPSUpgradeable,
    OwnableUpgradeable,
    ERC20Upgradeable
{
    ...
    function _authorizeUpgrade(address v) internal override onlyOwner {}
    ...
}

Use Ownable2StepUpgradeable instead of OwnableUpgradeable and override renounceOwnership() with implementation that reverts.

Malicious actor can initialize implementation contract of a proxy by calling initialize(). If the implementation contract makes a controlled delegatecall or performs self-destruct, it's possible to delete the implementation.