Fund loss possibility when bridge refunds to compromised owner account

Summary

The bridgePool() function uses owner() as a _refundRecipient, which might represent compromised account. In the event of bridge refund, the funds might be lost as sent to account under attacker control.

Vulnerability Detail

The SophonFarming has the bridgePool() function that can be called by anyone. This function calls the bridge.deposit() with owner() set as a _refundRecipient. However, the SophonFarming implements Ownable2Step, thus the owner transfer can be initiated in any moment, e.g. in the event of emergency as current owner priv keys were compromised. Assuming that solution owner initiated the owner transfer and set the pendingOwner, anyone can trigger the bridgePool() function before ownership transfer is ended. When the bridge deposit finish unsuccessfully, the _refundRecipient will still point to previous, compromised account.

Impact

Possible loss of pool's fund when refunded to compromised account.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L772

    function bridgePool(uint256 _pid) external {
        if (!isFarmingEnded() || !isWithdrawPeriodEnded() || isBridged[_pid]) {
            revert Unauthorized();
        }

        updatePool(_pid);
        PoolInfo storage pool = poolInfo[_pid];

        uint256 depositAmount = pool.depositAmount;
        if (depositAmount == 0 || address(bridge) == address(0) || pool.l2Farm == address(0)) {
            revert BridgeInvalid();
        }

        IERC20 lpToken = pool.lpToken;
        lpToken.approve(address(bridge), depositAmount);

        // TODO: change _refundRecipient, verify l2Farm, _l2TxGasLimit and _l2TxGasPerPubdataByte
        // These are pending the launch of Sophon testnet
        bridge.deposit(
            pool.l2Farm,            // _l2Receiver
            address(lpToken),       // _l1Token
            depositAmount,          // _amount
            200000,                 // _l2TxGasLimit
            0,                      // _l2TxGasPerPubdataByte
            owner()                 // _refundRecipient
        );

        isBridged[_pid] = true;

        emit Bridge(msg.sender, _pid, depositAmount);
    }

Tool used

Manual Review

Recommendation

It is recommended to either disallow usage of bridgePool() function when ownership transfer is in progress, or limit access to this function only for current owner.

sherlock-audit / 2024-05-sophon-judging

cocacola - Fund loss possibility when bridge refunds to compromised owner account #136