Socket App Chain Bridgeable Tokens Security Review Report

STATUS: Fixed

When transferring a message cross-chain from Vault on chain A to Vault on chain B, a user should pay for gas fees. As such, the functions are payable and fees should be submitted in msg.value and the gasLimit parameter when calling the function.

However, currently a user has to blindly determine the correct amount of fees to submit, which can be prone to mistakes.

The ISocket | Socket Data Layer interface has a function getMinFees() which would allow to determine correct fee amount and it can be checked inside of the function before sending the cross-chain message.

function withdrawFromAppChain(
    address receiver_,
    uint256 burnAmount_,
    uint256 gasLimit_,
    address connector_
) external payable {
    if (_burnLimitParams[connector_].maxLimit == 0)
        revert ConnectorUnavailable();
 
    _consumeFullLimit(burnAmount_, _burnLimitParams[connector_]); // reverts on limit hit
 
    totalMinted -= burnAmount_;
    token__.burn(msg.sender, burnAmount_);
 
    uint256 unlockAmount = exchangeRate__.getUnlockAmount(
        burnAmount_,
        connectorLockedAmounts[connector_]
    );
    connectorLockedAmounts[connector_] -= unlockAmount; // underflow revert expected
 
    IConnector(connector_).outbound{value: msg.value}(
        gasLimit_,
        abi.encode(receiver_, unlockAmount)
    );
 
    emit TokensWithdrawn(connector_, msg.sender, receiver_, burnAmount_);
}
 
function depositToAppChain(
    address receiver_,
    uint256 amount_,
    uint256 gasLimit_,
    address connector_
) external payable {
    if (_lockLimitParams[connector_].maxLimit == 0)
        revert ConnectorUnavailable();
 
    _consumeFullLimit(amount_, _lockLimitParams[connector_]); // reverts on limit hit
 
    token__.safeTransferFrom(msg.sender, address(this), amount_);
 
    IConnector(connector_).outbound{value: msg.value}(
        gasLimit_,
        abi.encode(receiver_, amount_)
    );
 
    emit TokensDeposited(connector_, msg.sender, receiver_, amount_);
}

Vault.sol and Controller.sol should implement a function allowing to estimate fees for the connector used.

STATUS: Fixed

The function to update the limit params in Controller and Vault directly updates the maxLimit and ratePerSecond variables in the connector’s LimitParams struct.

However, this function does not take the lastUpdateTimestamp into account and so it will be applied to the past as well.

The struct would previously already have the maxLimit and ratePerSecond defined and if they are directly updated, the already passed period of time will be counted using the new ratePerSecond, which may be higher or lower.

function updateLimitParams(
    UpdateLimitParams[] calldata updates_
) external onlyOwner {
    for (uint256 i; i < updates_.length; i++) {
        if (updates_[i].isLock) {
            _lockLimitParams[updates_[i].connector].maxLimit = updates_[i]
                .maxLimit;
            _lockLimitParams[updates_[i].connector]
                .ratePerSecond = updates_[i].ratePerSecond;
        } else {
            _unlockLimitParams[updates_[i].connector].maxLimit = updates_[i]
                .maxLimit;
            _unlockLimitParams[updates_[i].connector]
                .ratePerSecond = updates_[i].ratePerSecond;
        }
    }

The function should either call _consumePartLimit with a 0 amount first, or directly set the lastUpdateTimestamp and lastUpdateLimit to 0 for a full reset, whichever is desired.

STATUS: Fixed

There is no ability for a Plug to disconnect from a socket by calling function Socket.sol:connect() with address(0) as the inboundSwitchboard_ and outboundSwitchboard_ parameters.

Because of the checks inside Socket.sol:connect() (L155-L160) that revert on inbound/outbound switchboard with address(0). Furthermore, registerSwitchboardForSibling() doesn’t allow to register a switchboard with address(0).

As a result, the disconnect() function in the DataLayer usage example reverts.

function connect(
    uint32 siblingChainSlug_,
    address siblingPlug_,
    address inboundSwitchboard_,
    address outboundSwitchboard_
) external override {
    // only capacitor checked, decapacitor assumed will exist if capacitor does
    // as they both are deployed together always
    if (
        address(capacitors__[inboundSwitchboard_][siblingChainSlug_]) ==
        address(0) ||
        address(capacitors__[outboundSwitchboard_][siblingChainSlug_]) ==
        address(0)
    ) revert InvalidConnection();
    ...
}

Fix the disconnect() function in DataLayer usage example.

STATUS: Fixed

The function receiveInbound for both the Vault and Controller contract will try to mint or transfer the token from the limits. In case of limits, the remaining amount is added to the pending amount for the receiver. Both functions also do not check that the Connector (msg.sender) is registered.

It could be the case that the deployer of a connector contract ConnectorPlug.sol on some chain turns malicious. He/she knows that his/her connector will be used by some Vault.sol or Controller.sol soon. Instead of deploying the original ConnectorPlug.sol on address 0x13...37. he/she can deploy a malicious contract first using the Smart Contract Audits - Composable Security CREATE/CREATE2 trick.

Malicious contract calls Vault.sol:receiveInbound(), for the Vault.sol the attacker wants to drain. In the call arguments unlockAmount equals to the vault balance and the receiver address is under the attacker’s control.

Consequently, pendingUnlocks[msg.sender][receiver] will be set to a controlled unlockAmount value.

After that the malicious deployer destroys the malicious contract and deploys normal ConnectorPlug.sol on address 0x13...37.

When the target Vault.sol whitelists the connector by calling Vault.sol:updateLimitParams(). Malicious deployer drains the Vault by calling Vault.sol:unlockPendingFor().

function receiveInbound(bytes memory payload_) external override {
    (address receiver, uint256 unlockAmount) = abi.decode(
        payload_,
        (address, uint256)
    );
    ...
    if (pendingAmount > 0) {
        // add instead of overwrite to handle case where already pending amount is left
        pendingUnlocks[msg.sender][receiver] += pendingAmount;
        connectorPendingUnlocks[msg.sender] += pendingAmount;
        emit TokensPending(
            msg.sender,
            receiver,
            pendingAmount,
            pendingUnlocks[msg.sender][receiver]
        );
    }
    ...
}

Vault.sol and Controller.sol inside receiveInbound() should check that a caller is a registered connector similarly to the depositToAppChain() and unlockPendingFor() functions:

if (_lockLimitParams[msg.sender].maxLimit == 0)
    revert ConnectorUnavailable();

STATUS: Fixed

The ExchangeRate contract imports and implements Ownable2Step from the OpenZeppelin library but does not utilize any functionality or properties provided by the Ownable2Step contract.

Inclusion of this contract is therefore redundant and unnecessarily increases the bytecode size of the contract.

contract ExchangeRate is IExchangeRate, Ownable2Step {
    // chainId input needed? what else? slippage?
    function getMintAmount(
        uint256 lockAmount,
        uint256 /* totalLockedAmount */
    ) external pure returns (uint256 mintAmount) {
        return lockAmount;
    }
 
    function getUnlockAmount(
        uint256 burnAmount,
        uint256 /* totalLockedAmount */
    ) external pure returns (uint256 unlockAmount) {
        return burnAmount;
    }
}

If the Ownable2Step functionality is not needed, remove the import statement and the inheritance from Ownable2Step in the ExchangeRate contract declaration.

STATUS: Fixed

Some public storage variables are only set in the constructor and can then never be set again.

IHub public hub__;
ISocket public socket__;
ERC20 public token__;

Change the variables to immutable in favour of gas savings on each call to these external contracts.

STATUS: Fixed

In the Controller.sol and Vault.sol contracts the error LengthMismatch is declared on line 33 and 40 respectively, but is is never used.

error LengthMismatch();

Remove the unused error.

The ConnectorPlug contract has functions to both connect and disconnect from a socket.

However the ConnectorPlug.sol:disconnect() becomes redundant because the same effect can be achieved when calling ConnectorPlug.sol:connect() with address(0) as the switchboard_ parameter.

ConnectorPlug.sol should be deployed many times, therefore it’s crucial it has the smallest size possible.

function disconnect(address siblingPlug_) external onlyOwner {
    socket__.connect(
        siblingChainSlug,
        siblingPlug_,
        address(0),
        address(0)
    );
    emit ConnectorPlugDisconnected(siblingPlug_);
}

Remove ConnectorPlug.sol:disconnect() function in favour of smaller bytecode size and deployment gas costs.

The Gauge contract defines a struct LimitParams that is used in both Controller and Vault for connector specific limits. However, the struct is not optimally packed.

The field lastUpdateTimestamp is always set to block.timestamp, which could fit in a uint32. In that case, ratePerSecond could be reduced to at least uint224 or less such that both fields would fit into one slot. Both variables are retrieved on a call to _getCurrentLimit, so that would save one SLOAD on each inbound message.

Similarly, depending on specification of maximum values, both maxLimit and lastUpdateLimit could be stored as uint128 so they would fit into one slot and save another SLOAD on each inbound message.

struct LimitParams {
    uint256 lastUpdateTimestamp;
    uint256 ratePerSecond;
    uint256 maxLimit;
    uint256 lastUpdateLimit;
}

Both the Controller and Vault employ a limit to minting and unlocking transferred funds through a Connector.

These limits are global and thus shared by all users. As such, a malicious user could constantly keep sending the same tokens back and forth using the Connectors and forcefully congest the limit queue.

This would lower and completely block the bridge for other users as they would constantly get rate limited.

function _consumeFullLimit(
    uint256 amount_,
    LimitParams storage _params
) internal {
    uint256 currentLimit = _getCurrentLimit(_params);
    if (currentLimit >= amount_) {
        _params.lastUpdateTimestamp = block.timestamp;
        _params.lastUpdateLimit = currentLimit - amount_;
    } else {
        revert AmountOutsideLimit();
    }
}

Set up monitoring for these types of congestion attacks and set the Connector limits accordingly.