Socket Super Token & Vault Security Review Report

Status: Fixed

The SuperTokenVault allows bridging and receiving inbound messages. When bridging, the corresponding token is transferred from the user to the vault and stored there until an inbound message comes in.

The bridging process also allows for arbitrary calls to the receiver of the message, after the message's assets have been transferred.

This can be exploited by sending a small amount of tokens (say 1 wei) to the address of a token (e.g. an ERC20) and setting the execution payload to transfer(address,uint256) with the attacker's address and the vault's balance.

The message would be bridged and delivered to the receiving SuperTokenVault, where it would transfer 1 wei of the token to the token.

The Executor._execute() function should allow the execution of whitelisted payloads.

Status: Fixed

The SuperToken contract allows the minting of pending tokens for the receiver by calling the mintPendingFor() function. Similarly, the SuperTokenVault contract allows the unlocking and transferring of pending tokens for the receiver by calling the unlockPendingFor() function.

However, neither of these functions checks whether the receiver_ or the siblingChainSlug_ input parameters match pendingExecutions[identifier_].receiver and pendingExecutions[identifier_].siblingChainSlug.

As a result, an attacker could call the _execute() function with an arbitrary receiver_ parameter, using a payload pendingExecutions[identifier_].payload determined by the identifier_ value.

An attacker may choose to clean the pendingExecutions queue by triggering executions with identifier_ for random receivers.

This can lead to several consequences. Legitimate receivers won't receive the execution callback. Legitimate receivers may receive the execution callback, but with parameters intended for another receiver.

In another possible attack scenario, the perpetrator bridges tokens from Chain A to Chain B using the SuperTokenVault contract. They specify the transferFrom(address,address,uint256) function as the execution payload since arbitrary execution payloads are allowed. They can choose a victim who has approved an excessive amount of tokens to the same SuperTokenVault contract on Chain B. Suppose that the execution payload is cached inside pendingExecutions due to an unsuccessful execution caused by the perpetrator deliberately reverting the payload execution on Chain B.

Afterwards, they can specify a token address (e.g. of ERC20 token) as the receiver_ parameter when calling unlockPendingFor() and siphon the victim's tokens.

Add the following checks inside mintPendingFor() and unlockPendingFor().

+ require(receiver_ == pendingExecutions[identifier_].receiver);
+ require(siblingChainSlug_ == pendingExecutions[identifier_].siblingChainSlug);

The Executor._execute() function should allow the execution of whitelisted payloads.

The SuperTokenVault will hold all tokens that are bridged to another chain in escrow and so it will hold a significant value.

Similar to all other contracts in the protocol, this contract also has a rescue mechanism where someone with the RESCUE_ROLE is able to withdraw any token from the contract.

This leads to a centralisation risk in case an EAO with this role gets compromised and all assets could get lost.

function rescueFunds(
    address token_,
    address rescueTo_,
    uint256 amount_
) external onlyRole(RESCUE_ROLE) {
    RescueFundsLib.rescueFunds(token_, rescueTo_, amount_);
}

We would recommend to consider not allowing rescuing the SuperTokenVault's token and only other mistakenly sent tokens instead.

For example:

function rescueFunds(
    address token_,
    address rescueTo_,
    uint256 amount_
) external onlyRole(RESCUE_ROLE) {
    if (token_ == address(token__)) {
        revert NoRescueAllowed();
    }
    RescueFundsLib.rescueFunds(token_, rescueTo_, amount_);
}

Status: Fixed

The getMinFees() function currently considers the value 96 as the byte-length of the message transmitted cross-chain. This may lead to an incorrect evaluation of the minimum fee amount.

function getMinFees(
    uint32 siblingChainSlug_,
    uint256 msgGasLimit_
) external view returns (uint256 totalFees) {
    return
        socket__.getMinFees(
            msgGasLimit_,
            96,
            bytes32(0),
            bytes32(0),
            siblingChainSlug_,
            address(this)
        );
}

Consider adding a separate parameter, payloadLength_, to the getMinFees() function. This parameter will allow the user to specify the payload length, ensuring accurate calculation of the minimum fee amount.

The bridge() function in the SuperToken contract relies on the SocketPlug.getMinFees() function to help users estimate the fees they need to include as msg.value when calling bridge(). However, if a user fails to provide enough fees, the call reverts inside the SocketSrc contract at lines 178-191 - https://github.com/SocketDotTech/socket-DL/blob/master/contracts/socket/SocketSrc.sol#L178-L191.

To optimize gas during error scenarios, it may be beneficial to add an earlier check inside the bridge() function to verify if the user has provided enough fees.

Status: Fixed

The _execute() function does not utilize the return value from the excessivelySafeCall() function. As a result, the variable MAX_COPY_BYTES could be set to 0, which would help to save gas.

function _execute(
    address target_,
    bytes memory payload_
) internal returns (bool success) {
    (success, ) = target_.excessivelySafeCall(
        gasleft(),
        MAX_COPY_BYTES,
        payload_
    );
}

uint16 private constant MAX_COPY_BYTES = 150;

Set MAX_COPY_BYTES to 0.

Status: Fixed

The SuperTokenVault uses Solmate's SafeTransferLib to handle ERC20 transfers.

However, this library does not check whether the target token contract actually exists. If it does not exist, then the functions will simply return successfully and the SuperTokenVault will think that the transfer was completed.

Since the token is only set during construction of the SuperTokenVault, this only becomes a security risk if the SuperTokenVault is deployed before the actual token is deployed.

This could happen if SuperTokenVaults are deployed using factory contracts and the target token can be pre-calculated (e.g. a new token from a protocol that always uses the same deploy EOA or a deterministic wrapper address of a bridge).

import "solmate/utils/SafeTransferLib.sol";
 
contract SuperTokenVault is Gauge, ISuperTokenOrVault, AccessControl, Execute {
    using SafeTransferLib for ERC20;
    [..]
}

The vulnerable scenario is very rare, however it could become more probable if there is a factory or automatic process ofTSuperTokenVault creation.

To protect against this, OpenZeppelin's Address.isContract could be used to check the existence of the token contract in the constructor, which is only a small price.

The SocketPlug contract takes care of connecting with a Socket through the connect and disconnect function.

The connect function will save the sibling plug address for the sibling chain slug in a mapping, however the disconnect function does not clear this mapping even though the sibling plug address is set to address zero.

Also delete the mapping entry in disconnect.

for example:

delete siblingPlugs[siblingChainSlug_];