Socket Supermodular Contracts Security Review Report

The contract YieldToken has a permit function, allowing token approvals to be made via signatures. The signature incorporates a token amount equivalent to convertToShares(value), meaning that value is supposed to be in underlying assets, yet it effectively provides approval for the specified value, and allowance is expressed in shares.

This means that the effective approval value will always be higher than the user meant and signed for. It may lead to user fund loss, when the user plans to approve X amount, but will approve more.

Consider changing this line of code in the permit() function:

allowance[recoveredAddress][spender] = convertToShares(value);

In the maxWithdraw function within the YieldToken contract, where convertToAssets is called twice on _balanceOf[owner], leading to an incorrect calculation of the maximum amount of underlying assets that can be withdrawn. Similarly, the maxRedeem function currently does not align with typical expectations for such a function in an investment or yield token contract, as it also incorrectly calculates the redeemable amount due to improper use of convertToAssets.

    function maxWithdraw(address owner) public view virtual returns (uint256) {
        return convertToAssets(convertToAssets(_balanceOf[owner]));
    }
 
    function maxRedeem(address owner) public view virtual returns (uint256) {
        return convertToAssets(_balanceOf[owner]);
    }

If the described logic is correct change the maxWithdraw and maxRedeem functions as follows:

    function maxWithdraw(address owner) public view virtual returns (uint256) { // @audit-issue
        return convertToAssets(_balanceOf[owner]);
    }
 
    function maxRedeem(address owner) public view virtual returns (uint256) {
        return _balanceOf[owner];
    }

In the function YieldToken::calculateMintAmount(), the minted shares are determined using a round-up calculation. However, this rounding method introduces a vulnerability, allowing an attacker to mint shares with a minimal amount of tokens and subsequently steal tokens from other users.

For instance, consider the following scenario:

• totalUnderlyingAssets = 100
• supply = 10 If an attacker uses 1 token, they will obtain a number of shares calculated as follows:
• calculateMintAmount() = ceil(1 * 10 / 100) = 1 Upon burning 1 share of tokens, the attacker can acquire:
• convertToAssets() = floor(1 * (100 + 1) / (10 + 1)) = 9 By employing this strategy, the attacker gains a profit of 9 - 1 = 8 tokens.

Consider using the round-down calculation to calculate the minted shares.

The Vault::bridge() function facilitates the transfer of tokens from the current chain to another chain. Initially, users must transfer their tokens to the contract. Subsequently, the connector.outbound() function is triggered to relay the message to the destination chain. However, successful delivery of the message from the socket to the connector on the destination chain is not guaranteed due to factors such as gasLimit or fees: Message Failure & Retry | Socket Data Layer .

If the message delivery fails on the destination chain, there may be a need to resend the message from the source chain. The issue arises when the message cannot be delivered on the destination chain, resulting in users' tokens transferred to the vault on the source chain not being refunded. Consequently, users' funds become locked within the contract.

We would recommend to consider implement a rescue or cancellation mechanism for stuck transfers.

The limit imposed by LimitHook could be circumvented. Imagine a scenario where a Controller or Vault has connectors for various chains (e.g., Optimism and Arbitrum) and utilizes the LimitHook contract to enforce daily limits.

The issue arises from the fact that identifierCache does not take into account the connector value, only incorporating the receiver and pending amount:

identifierCache = abi.encode(updatedReceiver, pendingAmount)

When a user receives tokens from Optimism and reaches the daily limit, an entry is made in the identifierCache mapping for the message id with the value abi.encode(params_.transferInfo.receiver, pendingAmount). Consequently, the user can later retry the message id once the daily limit resets for Optimism.

Instead, the user could invoke Controller.retry() with a connector for a different chain (Arbitrum) where the limit has not been reached yet, specifying the message id received from Optimism.

This action will trigger _limitDstHook() for a different connector (Arbitrum) but with the pendingMint received from Optimism:

(uint256 consumedAmount, uint256 pendingAmount) = _limitDstHook(
    params_.connector,
    pendingMint
);

Incorporate the connector value into identifierCache and verify within preRetryHook() whether params_.connector matches the connector decoded from the identifierCache value.

The lastSyncTimestamp variable in the YieldToken contract is only set but never used for any aspect of the code.

As of right now, the variable serves no purpose and should therefore not be stored on the blockchain. If the timestamp is required for front-end purposes, the timestamp could be stored off-chain or emitted in an event and tracked off-chain.

Removing the variable would reduce gas costs by one SSTORE (5.000 gas) for each call to updateTotalUnderlyingAssets. At an ETH price of $4000 and gas price of 80 Gwei that equals and extra ~$1.60 per call.

Unused variables should be removed, this will reduce gas costs for storage and computation.

YieldTokenBase.permit() uses require statements instead of custom errors.

Replace the statements with custom errors to reduce the deployment gas and make the error-handling technique more consistent.

The function Base._afterBridge() uses an implicit approach to check whether the ETH amount received is bigger than the amount bridged: msg.value - transferInfo.amount during the fee calculation will revert with an underflow error otherwise. It is recommended that an explicit check with a custom error be added to improve the UX.

The function Base::updateHook() is utilized to update the new hook address for the hub. This function grants the new hook_ address an unlimited allowance of tokens. However, it does not revoke the allowance of the contract with the old hook yet. This failure to revoke can lead to a scenario where the old hook, if controlled by malicious actors or hacked, can still steal assets from hub contracts through the allowance, even if the new hook is set for the hub.

Consider revoking the allowance associated with the old hook in the updateHook function.

In each pre-hook call function, the notShutDown modifier is used to check the shutdown status of the hub. However, the post-hook call function also contains the same modifier, making it redundant for these functions, assuming that the flow will always be that these are called in pairs from the Vault/Controller.

The only exception is srcPreHookCall in Controller_YieldLimitExecHook because it doesn't have the notShutdown modifier (srcPostHookCall does).

Remove the notShutDown modifier in the post-hook call function.