1 Executive Summary
This report presents the results of our engagement with Socket.tech to review the smart contracts component of the system.
The review was conducted over three weeks, from February 13th, 2023 to March, 3rd 2023, by David Oz and George Kobakhidze. A total of 30 person-days were spent.
The assessment was focused on the core parts of the Socket system, including its SocketGateway, Routes, and Controllers architecture. While Socket aims to aggregate a multitude of bridges and decentralized exchanges, this audit focused only on a few specific integrations - Celer bridge, Stargate bridge, and 1inch DEX.
The initial commit hash was a8d0ad1c280a699d88dc280d9648eacaf215fb41, which was then switched to d0841a3e96b54a9d837d2dba471aa0946c3c8e7b after three days.
2 Scope
Our review focused on the commit hash d0841a3e96b54a9d837d2dba471aa0946c3c8e7b. The list of files in scope can be found in the Appendix.
2.1 Objectives
Together with the Socket.tech team, we identified the following priorities for our review:
- Assess if the system is implemented consistently with the intended functionality, and without unintended edge cases, such as:
- The system is modular and flexible enough to add new integrations and controllers.
- The system is resilient against users giving infinite approvals.
- Route id based verifiability of calldata on integrators’ end.
- Only privileged role holders can add new and pause existing routes.
- Only intended addresses should receive swapped or bridged funds.
- Identify known vulnerabilities particular to smart contract systems, as outlined in our Smart Contract Best Practices, and the Smart Contract Weakness Classification Registry.
3 Recommendations
3.1 Consider Adding a Task to CI/CD to Verify That Future Delegatee Contracts Are Safe
Description
The system is based on delegating calls to routes and controllers. delegatee contracts should not write to storage, self-destruct, or delegate-call to unknown contracts. In addition, caution is needed when using msg.value as mentioned in issue 6.13. We were not able to find any concrete instances of the described issue, however, we do see how these pitfalls may become an issue in future delegatee contracts.
Recommendation
Consider using a package like https://github.com/OpenZeppelin/openzeppelin-upgrades or an equivalent.
3.2 Consider Using a Package Manager Instead of Vendor Code
Description
When you vendor a library, you essentially copy the library code into your project’s codebase, which can lead to problems with version control and code management. If the vendor library code changes, you’ll need to manually update the code in your project, which can be time-consuming and error-prone. Additionally, if the vendor library has any security vulnerabilities, copying the code into your project can make it more difficult to address those vulnerabilities. Instead, it’s generally better to use the Solidity package manager, which allows you to import libraries into your project without copying their code. This way, you can easily update the library code, and any security vulnerabilities can be addressed centrally.
Errors that arise when copying interfaces can have far-reaching consequences, particularly when it comes to functions that are mislabeled as view or pure in the interface but are actually state-changing functions. If such functions are called they have the potential to cause an entire transaction to revert, leading to a potential denial of service.
Examples
src/libraries/LibBytes.sol:L4
library LibBytes {
src/libraries/Pb.sol:L6
library Pb {
3.3 Consider Adding Non-Reentrant Modifiers to State-Changing Functions in SocketGateway
Description
SocketGateway is the main contract for user interaction in the system. The contract is designed to be used by multiple users where the main flow is that a user is depositing funds to the contract and chooses how these funds should be used by external contracts. Currently, we were not able to find any concrete issues that are caused by reentrancies, however, given the fact that the system is planned to be expanded by the use of delegatecalls, it is recommended to add nonReentrant modifiers to state changing functions inside SocketGateway.
3.4 Use the Same Solidity Version Across Contracts
Description
Most contracts use the same range for Solidity versions with pragma solidity ^0.8.4. There are also many that use pragma solidity >=0.8.0.
Recommendation
Lock in a specific version of solidity or at least pick a consistent range. This would help avoid any issues and inconsistencies that may arise in deploying the various smart contracts across different Solidity versions.
3.5 Gas Optimizations
Resolution
Description
The client mentioned that gas was important. Optimizing for gas should never come at the cost of security. However, we noticed a few optimizations that could be made.
Examples
socketGateway can be replaced with address(this)
src/bridges/cbridge/CelerImpl.sol:L320
tokenInstance.safeTransferFrom(msg.sender, socketGateway, amount);
src/bridges/cbridge/CelerImpl.sol:L412
uint256 _initialBalanceTokenOut = socketGateway.balance;
src/bridges/cbridge/CelerImpl.sol:L417
if (request.receiver != socketGateway) {
src/bridges/cbridge/CelerImpl.sol:L430
if (socketGateway.balance > _initialBalanceTokenOut) {
As discussed with the client, resetting the approval to zero after the swap is implemented to prevent a future USDT approval from reverting in case the previous swap didn’t consume the entire allowance (as the USDT contract requires resetting the allowance to zero, before changing it). However, this should not happen in a system that behaves properly. To save gas, we recommend removing this check and implementing a gateway function that allows setting a token allowance to zero. Note that this function does not need to be protected, as it only allows setting the gateway token allowance to zero.
src/swap/oneinch/OneInchImpl.sol:L64
token.safeApprove(ONEINCH_AGGREGATOR, 0);
src/swap/oneinch/OneInchImpl.sol:L123
token.safeApprove(ONEINCH_AGGREGATOR, 0);
3.6 Duplicated Code
Description
Duplicate code, or code that is copied and pasted multiple times within a project or across projects, is generally not a good practice in software development. It can lead to several issues, including increased maintenance costs, decreased code readability, and a higher likelihood of introducing bugs into the codebase. When code is duplicated, any changes that need to be made must be replicated across all instances of the code, which can be time-consuming and error-prone. Additionally, duplicated code can make it harder to understand the overall structure of the codebase, as well as make it more difficult to identify and fix issues when they arise.
Examples
FeesTakerController- functions in this contract share similar logic that can be de-duplicated.OneInchImpl- functions in this contract share similar logic that can be de-duplicated.CelerImpl- functions in this contract share similar logic that can be de-duplicated.Stargate L1,Stargate L2- the contracts themselves are pretty similar.SwapImplBase,BridgeImplBase- the contracts themselves are pretty similar.
Recommendation
To mitigate these problems, it’s often better to refactor duplicated code into reusable functions or classes or to find other ways to modularize the code and reduce redundancy. By doing so, code can be more easily maintained, tested, and extended over time, leading to a more robust and reliable software application.
4 System Overview
The Socket system of contracts aims to provide its users with an easy means to access a multitude of common and relatively interchangeable solutions, such as bridges and swaps. Essentially, the system aggregates access to different systems to just the one SocketGateway contract that performs delegatecall transactions into routes, implementation contracts that hold logic necessary to interact with integrated solutions. This is done by managing contract addresses as routes in the registry maintained in the SocketGateway that is able to easily (although with Owner administrative power only) add and disable them but never change an existing one. Hence, the name of the system - Socket. The Socket team can create and remove routes but can’t change the logic within already registered ones.
The contracts are written with a high emphasis on immutability and gas efficiency, trying to minimize state variables and changes to them as much as possible. Even the deployment of future integrations is designed to be more gas efficient by pre-populating addresses of implementation contracts that can be derived through the CREATE2 opcode in the SocketDeployFactory, as described more below.
As users interact with the system, they should take note of what routes, and therefore the associated bridge or swap solutions, they will be interacting with. The Socket system adds a helpful aggregated wrapper around these destinations, but it does not improve upon their own security. In fact, since the route IDs will always refer to the same address they were assigned to, and the contracts at these addresses have either the same logic as they did in the beginning or are disabled, users and systems integrating Socket could come up with a whitelist of route IDs to ensure they always go through those solutions that they deem to be trusted.
Below you may find an overview diagram of the system’s scope for this audit and small descriptions of major components:
4.1 SocketGateway
This is the entry point into the whole Socket system. Both users and Socket team via the SocketGateway’s Owner will call into this contract to interact with this system. It contains the registry of all route IDs and their associated implementation contracts.
4.2 SocketDeployFactory
In order to be more efficient, the Socket team decided to pre-populate 512 route ID addresses by calculating them via the CREATE2 opcode. This deployment pattern is done through the SocketDeployFactory, that is also able to destroy and disable existing routes.
4.3 BridgeImplBase & SwapImplBase
The two common types of routes, BridgeImplBase and SwapImplBase are base contracts that implement default functionality for routes, such as isSocketGatewayOwner modifiers, token rescue functions, and virtual use-case-specific functions to be re-implemented for bridge and swap routes.
4.4 BaseController
This is the base contract that implements basic functionality required for the controllers. Unlike routes that lead into bridges and swap solutions, controllers may have more complex logic, such as minimal refueling of native tokens for the recipient address and implementing extra fees.
4.5 ***StorageWrapper
Some specific cases, like the Celer bridge, require storage of additional variables. Since routes and controllers don’t have storage that is used by the SocketGateway due to the usage of delegatecall, additional storage wrapper contracts need to be deployed to keep track of extra varaibles.
4.6 Integrated Solution (1inch, Celer, Stargate etc.)
These are the downstream solutions that are aggregated by Socket and perform the requested task for the users, such as swaps and bridge transfers.
5 Security Specification
This section describes, from a security perspective, the expected behavior of the system under audit. It is not a substitute for documentation. The purpose of this section is to identify specific security properties that were validated by the audit team.
The primary security concept for the Socket system is that it is an aggregator contract. Socket provides a way for users to interact with many other solutions, but, in its current form, it does not provide any additional security guarantees for the systems it integrates. The security is wholly inherited for bridge transfers and swaps that it performs as the data payloads that users provide are simply passed through to the downstream solutions.
The Socket system itself does have an emphasis on the immutability of the contracts it deploys, however. The contract addresses inserted in its registry can be, at most, disabled, but they can’t have their logic be changed to something else. There are a few admin-controlled functions which are mostly isolated to adding new routes and controllers, but there are exceptions where SocketGateway Owner addresses may have privileged access into the control and data flow of route logic, like the case with the Celer bridge. In the current scope this access is limited to refund and failure recovery cases.
5.1 Actors
The relevant actors are listed below:
- The Socket team
- The integrated systems
- End users & integrating systems
5.2 Trust Model
In any system, it’s important to identify what trust is expected/required between various actors. For this audit, we established the following trust model:
Socket team:
In the current system, the Socket team is responsible for several critical components to ensure the system’s correct behavior.
First, the team quite simply has administrative abilities over the SocketGateway and its routes via the onlyOwner modifier. These include management functions like addRoute, addController, disableRoute, and disableController, as well as the ability to pull out stuck tokens from the contracts via rescueEther and rescueFunds. As referred to in one of the filed issues, the ability to add arbitrary logic via addRoute and addController that will execute via delegatecall from SocketGateway as msg.sender may create edge cases where the Owner address can actually have access to user funds and can steal them, like the case with the Celer bridge refunds.
Second, the Socket team also provides and connects to APIs that compile the payloads necessary for correct route execution. These payloads are encoded (although sometimes in a simple manner) as determined by the route’s integrated system and are not always easy for users to understand nor necessarily checked against other user-provided inputs, such as swap amounts. This, however, can be dealt with by more advanced users by compiling the necessary payloads themselves, although it is unlikely to be the common use case
As a result, the Socket team itself is trusted to at least set up the appropriate routes and not have its privileged access compromised in certain edge cases for the system to function correctly.
Integrated systems:
The systems that Socket routes to likewise play a crucial role since they are what is being aggregated. They are trusted to continue operating as assumed by their specifications (such as implementing swaps and bridge transfers correctly) and to treat the Socket system as any other smart contract using these solutions. As a result, their security assumptions are inherited in the Socket system as well.
End users
Finally, the end users are the actors that make the system go. They provide the funds that go through the system and hop between different routes. Due to the nature of the solutions that Socket integrates, the end users’ interactions with the gateway start and end with a single transaction in the vast majority of the cases, the exceptions being cases like refund scenarios with specific bridges. As a result, the users need not act in any specific way or perform any duties beyond their initial transactions. Similarly, other systems can act as end users themselves and use and integrate the Socket system into their processes. In fact, due to the nature of the Socket route and controller IDs, these systems can assess and compile whitelists of IDs to ensure that only certain DEXs/bridges/logic are executed through this system, so there is some granularity as to how the Socket system can be integrated.
5.3 Security Properties
The following is a non-exhaustive list of security properties that were assessed in this audit:
System immutability
The routes and controllers that are maintained in the SocketGateway registry can be disabled but are immutable otherwise. No new code can be added in their place, with the exception being a revert-only option.
Contract balances non-increasing
The SocketGateway and its associated routes and controllers are not meant to hold any tokens by design. This, however, can sometimes fail due to refund mechanisms of specific bridges as well as due to incorrectly submitted payloads for swap routes. The system will not benefit or interact with stuck tokens in any way, and there are rescue onlyOwner functions that could help retrieve the tokens. Nonetheless, there are cases when these stuck tokens, particularly native chain tokens, could actually be stolen by malicious actors, as referred to in a few filed issues. It is recommended to pay close attention to token balances on the gateway and rescue them swiftly.
6 Findings
Each issue has an assigned severity:
- Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
- Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
- Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
- Critical issues are directly exploitable security vulnerabilities that need to be fixed.
6.1 Funds Refunded From Celer Bridge Might Be Stolen Major
Resolution
Description
The function refundCelerUser from CelerImpl.sol allows a user that deposited into the Celer pool on the source chain, to be refunded for tokens that were not bridged to the destination chain. The tokens are reimbursed to the user by calling the withdraw method on the Celer pool. This is what the refundCelerUser function is doing.
src/bridges/cbridge/CelerImpl.sol:L413-L415
if (!router.withdraws(transferId)) {
router.withdraw(_request, _sigs, _signers, _powers);
}
From the point of view of the Celer bridge, the initial depositor of the tokens is the SocketGateway. As a consequence, the Celer contract transfers the tokens to be refunded to the gateway. The gateway is then in charge of forwarding the tokens to the initial depositor. To achieve this, it keeps a mapping of unique transfer IDs to depositor addresses. Once a refund is processed, the corresponding address in the mapping is reset to the zero address.
Looking at the withdraw function of the Celer pool, we see that for some tokens, it is possible that the reimbursement will not be processed directly, but only after some delay. From the gateway point of view, the reimbursement will be marked as successful, and the address of the original sender corresponding to this transfer ID will be reset to address(0).
if (delayThreshold > 0 && wdmsg.amount > delayThreshold) {
_addDelayedTransfer(wdId, wdmsg.receiver, wdmsg.token, wdmsg. // <--- here
} else {
_sendToken(wdmsg.receiver, wdmsg.token, wdmsg.
}
It is then the responsibility of the user, once the locking delay has passed, to call another function to claim the tokens. Unfortunately, in our case, this means that the funds will be sent back to the gateway contract and not to the original sender. Because the gateway implements rescueEther, and rescueFunds functions, the admin might be able to send the funds back to the user. However, this requires manual intervention and breaks the trustlessness assumptions of the system. Also, in that case, there is no easy way to trace back the original address of the sender, that corresponds to this refund.
However, there is an additional issue that might allow an attacker to steal some funds from the gateway. Indeed, when claiming the refund, if it is in ETH, the gateway will have some balance when the transaction completes. Any user can then call any function that consumes the gateway balance, such as the swapAndBridge from CelerImpl, to steal the refunded ETH. That is possible as the function relies on a user-provided amount as an input, and not on msg.value.
Additionally, if the refund is an ERC-20, an attacker can steal the funds by calling bridgeAfterSwap or swapAndBridge from the Stargate or Celer routes with the right parameters.
src/bridges/cbridge/CelerImpl.sol:L120-L127
function bridgeAfterSwap(
uint256 amount,
bytes calldata bridgeData
) external payable override {
CelerBridgeData memory celerBridgeData = abi.decode(
bridgeData,
(CelerBridgeData)
);
src/bridges/stargate/l2/Stargate.sol:L183-L186
function swapAndBridge(
uint32 swapId,
bytes calldata swapData,
StargateBridgeDataNoToken calldata stargateBridgeData
Note that this violates the security assumption: “The contracts are not supposed to hold any funds post-tx execution.”
Recommendation
Make sure that CelerImpl supports also the delayed withdrawals functionality and that withdrawal requests are deleted only if the receiver has received the withdrawal in a single transaction.
6.2 Calls Made to Non-Existent/Removed Routes or Controllers Will Not Result in Failure Major
Resolution
disabledRouteAddress contract to be returned for disabled routes instead of a address(0).
Description
This issue was found in commit hash a8d0ad1c280a699d88dc280d9648eacaf215fb41.
In the Ethereum Virtual Machine (EVM), delegatecall will succeed for calls to externally owned accounts and more specifically to the zero address, which presents a potential security risk. We have identified multiple instances of delegatecall being used to invoke smart contract functions.
This, combined with the fact that routes can be removed from the system by the owner of the SocketGateway contract using the disableRoute function, makes it possible for the user’s funds to be lost in case of an executeRoute transaction (for instance) that’s waiting in the mempool is eventually being front-ran by a call to disableRoute.
Examples
src/SocketGateway.sol:L95
(bool success, bytes memory result) = addressAt(routeId).delegatecall(
src/bridges/cbridge/CelerImpl.sol:L208
.delegatecall(swapData);
src/bridges/stargate/l1/Stargate.sol:L187
.delegatecall(swapData);
src/bridges/stargate/l2/Stargate.sol:L190
.delegatecall(swapData);
src/controllers/BaseController.sol:L50
.delegatecall(data);
Even after the upgrade to commit hash d0841a3e96b54a9d837d2dba471aa0946c3c8e7b, the following bug is still present:
To optimize gas usage, the addressAt function in socketGateway uses a binary search in a hard-coded table to resolve a routeID (routeID <= 512) to a contract address. This is made possible thanks to the factory using the CREATE2 pattern. This allows to pre-compute future addresses of contracts before they are deployed. In case the routeID is strictly greater than 512, addressAt falls back to fetching the address from a state mapping (routes).
The new commit hash adds a check to make sure that the call to the addressAt function reverts in case a routeID is not present in the routes mapping. This prevents delegate-calling to non-existent addresses in various places of the code. However, this does not solve the issue for the hard-coded route addresses (i.e., routeID <= 512). In that case, the addressAt function still returns a valid route contract address, despite the contract not being deployed yet. This will result in a successful delegatecall later in the code and might lead to various side-effects.
src/SocketGateway.sol:L411-L428
function addressAt(uint32 routeId) public view returns (address) {
if (routeId < 513) {
if (routeId < 257) {
if (routeId < 129) {
if (routeId < 65) {
if (routeId < 33) {
if (routeId < 17) {
if (routeId < 9) {
if (routeId < 5) {
if (routeId < 3) {
if (routeId == 1) {
return
0x822D4B4e63499a576Ab1cc152B86D1CFFf794F4f;
} else {
return
0x822D4B4e63499a576Ab1cc152B86D1CFFf794F4f;
}
} else {
src/SocketGateway.sol:L2971-L2972
if (routes[routeId] == address(0)) revert ZeroAddressNotAllowed();
return routes[routeId];
Recommendation
Consider adding a check to validate that the callee of a delegatecall is indeed a contract, you may refer to the Address library by OZ.
6.3 Owner Can Add Arbitrary Code to Be Executed From the SocketGateway Contract Medium
Resolution
The client team has responded with the following note:
Noted, we will setup tests and rigorous processes around adding new routes.
Description
The Socket system is managed by the SocketGateway contract that maintains all routes and controller addresses within its state. There, the address with the Owner role of the SocketGateway contract can add new routes and controllers that would have a delegatecall() executed upon them from the SocketGateway so user transactions can go through the logic required for the bridge, swap, or any other solution integrated with Socket. These routes and controllers would then have arbitrary code that is entirely up to the Owner, though users are not required to go through any specific routes and can decide which routes to pick.
Since these routes are called via delegatecall(), they don’t hold any storage variables that would be used in the Socket systems. However, as Socket aggregates more solutions, unexpected complexities may arise that could require storing and accessing variables through additional contracts. Those contracts would be access control protected to only have the SocketGateway contract have the privileges to modify its variables.
This together with the Owner of the SocketGateway being able to add routes with arbitrary code creates an attack vector where a compromised address with Owner privileges may add a route that would contain code that exploits the special privileges assigned to the SocketGateway contract for their benefit.
For example, the Celer bridge needs extra logic to account for its refund mechanism, so there is an additional CelerStorageWrapper contract that maintains a mapping between individual bridge transfer transactions and their associated msg.sender:
src/bridges/cbridge/CelerImpl.sol:L145
celerStorageWrapper.setAddressForTransferId(transferId, msg.sender);
src/bridges/cbridge/CelerStorageWrapper.sol:L6-L12
/**
* @title CelerStorageWrapper
* @notice handle storageMappings used while bridging ERC20 and native on CelerBridge
* @dev all functions ehich mutate the storage are restricted to Owner of SocketGateway
* @author Socket dot tech.
*/
contract CelerStorageWrapper {
Consequently, this contract has access-protected functions that may only be called by the SocketGateway to set and delete the transfer IDs:
src/bridges/cbridge/CelerStorageWrapper.sol:L32
function setAddressForTransferId(
src/bridges/cbridge/CelerStorageWrapper.sol:L52
function deleteTransferId(bytes32 transferId) external {
A compromised Owner of SocketGateway could then create a route that calls into the CelerStorageWrapper contract and updates the transfer IDs associated addresses to be under their control via deleteTransferId() and setAddressForTransferId() functions. This could create a significant drain of user funds, though, it depends on a compromised privileged Owner address.
Recommendation
Although it may indeed be unlikely, for aggregating solutions it is especially important to try and minimize compromised access issues. As future solutions require more complexity, consider architecting their integrations in such a way that they require as few administrative and SocketGateway-initiated transactions as possible. Through conversations with the Socket team, it appears that solutions such as timelocks on adding new routes are being considered as well, which would help catch the problem before it appears as well.
6.4 Dependency on Third-Party APIs to Create the Right Payload Medium
Resolution
The client team has responded with the following note:
We offset this risk by following 2 approaches - verifying oneinch calldata on our api before making full calldata for SocketGateway and making verifier contracts/libs that integrators can use to verify our calldata on their side before making actual transaction.
Description
The Socket system of routes and controllers integrates swaps, bridges, and potentially other solutions that are vastly different from each other. The function arguments that are required to execute them may often seem like a black box of a payload for a typical end user. In fact, even when users explicitly provide a destination token with an associated amount for a swap, these arguments themselves might not even be fully (or at all) used in the route itself. Instead, often the routes and controllers accept a bytes payload that contains all the necessary data for its action. These data payloads are generated off-chain, often via centralized APIs provided by the integrated systems themselves, which is understandable in isolation as they have to be generated somewhere at some point. However, the provided bytes do not get checked for their correctness or matching with the other arguments that the user explicitly provided. Even the events that get emitted refer to the individual arguments of functions as opposed to what actually was being used to execute the logic.
For example, the implementation route for the 1inch swaps explicitly asks the user to provide fromToken, toToken, amount, and receiverAddress, however only fromToken and amount are used meaningfully to transfer the amount to the SocketGateway and approve the fromToken to be spent by the 1inch contract. Everything else is dictated by swapExtraData, including even the true amount that is getting swapped. A mishap in the API providing this data payload could cause much less of a token amount to be swapped, a wrong address to receive the swap, and even the wrong destination token to return.
src/swap/oneinch/OneInchImpl.sol:L59-L63
// additional data is generated in off-chain using the OneInch API which takes in
// fromTokenAddress, toTokenAddress, amount, fromAddress, slippage, destReceiver, disableEstimate
(bool success, bytes memory result) = ONEINCH_AGGREGATOR.call(
swapExtraData
);
Even the event at the end of the transaction partially refers to the explicitly provided arguments instead of those that actually facilitated the execution of logic
src/swap/oneinch/OneInchImpl.sol:L84-L91
emit SocketSwapTokens(
fromToken,
toToken,
returnAmount,
amount,
OneInchIdentifier,
receiverAddress
);
As Socket aggregates other solutions, it naturally incurs the trust assumptions and risks associated with its integrations. In some ways, they even stack on top of each other, especially in those Socket functions that batch several routes together – all of them and their associated API calls need to return the correct payloads. So, there is an opportunity to minimize these risks by introducing additional checks into the contracts that would verify the correctness of the payloads that are passed over to the routes and controllers. In fact, creating these payloads within the contracts would allow other systems to integrate Socket more simpler as they could just call the functions with primary logical arguments such as the source token, destination token, and amount.
Recommendation
Consider allocating additional checks within the route implementations that ensure that the explicitly passed arguments match what is being sent for execution to the integrated solutions, like in the above example with the 1inch implementation.
6.5 NativeOptimismImpl - Events Will Not Be Emitted in Case of Non-Native Tokens Bridging Medium
Resolution
Description
In the case of the usage of non-native tokens by users, the SocketBridge event will not be emitted since the code will return early.
Examples
src/bridges/optimism/l1/NativeOptimism.sol:L110
function bridgeAfterSwap(
src/bridges/optimism/l1/NativeOptimism.sol:L187
function swapAndBridge(
src/bridges/optimism/l1/NativeOptimism.sol:L283
function bridgeERC20To(
Recommendation
Make sure that the SocketBridge event is emitted for non-native tokens as well.
6.6 Inconsistent Comments Minor
Resolution
Description
Some of the contracts in the code have incorrect developer comments annotated for them. This could create confusion for future readers of this code that may be trying to maintain, audit, update, fork, integrate it, and so on.
Examples
src/bridges/stargate/l2/Stargate.sol:L174-L183
/**
* @notice function to bridge tokens after swap. This is used after swap function call
* @notice This method is payable because the caller is doing token transfer and briding operation
* @dev for usage, refer to controller implementations
* encodedData for bridge should follow the sequence of properties in Stargate-BridgeData struct
* @param swapId routeId for the swapImpl
* @param swapData encoded data for swap
* @param stargateBridgeData encoded data for StargateBridgeData
*/
function swapAndBridge(
This is the same comment as bridgeAfterSwap, whereas it instead does swapping and bridging together
src/bridges/cbridge/CelerStorageWrapper.sol:L24-L32
/**
* @notice function to store the transferId and message-sender of a bridging activity
* @notice This method is payable because the caller is doing token transfer and briding operation
* @dev for usage, refer to controller implementations
* encodedData for bridge should follow the sequence of properties in CelerBridgeData struct
* @param transferId transferId generated during the bridging of ERC20 or native on CelerBridge
* @param transferIdAddress message sender who is making the bridging on CelerBridge
*/
function setAddressForTransferId(
This comment refers to a payable property of this function when it isn’t.
src/bridges/cbridge/CelerStorageWrapper.sol:L45-L52
/**
* @notice function to store the transferId and message-sender of a bridging activity
* @notice This method is payable because the caller is doing token transfer and briding operation
* @dev for usage, refer to controller implementations
* encodedData for bridge should follow the sequence of properties in CelerBridgeData struct
* @param transferId transferId generated during the bridging of ERC20 or native on CelerBridge
*/
function deleteTransferId(bytes32 transferId) external {
This comment is copied from the above function when it does the opposite of storing - it deletes the transferId
Recommendation
Adjust comments so they reflect what the functions are actually doing.
6.7 Ether Might Be Sent to Routes by Mistake, and Can Be Stolen Minor
Resolution
The client team has responded with the following note:
This can happen only if there is an error in API or integration. There are test cases to verify value on API side and we also run an automated testing suite using small amounts after each upgrade to the API before releasing to public. We also work with integrators to test out the flow covering all edge cases before they release. Overall we are fine with taking this risk and relying on rescue function to recover funds while testing.
Description
Most functions of SocketGateway are payable, and can receive ether, which is processed in different ways, depending on the routes. A user might send ether to a payable function of SocketGateway with a wrong payload, either by mistake or because of an API bug. Let’s illustrate the issue with the performAction of the 1inch route. However, this can be generalized to other routes.
src/SocketGateway.sol:L90-L97
function executeRoute(
uint32 routeId,
bytes calldata routeData,
bytes calldata eventData
) external payable returns (bytes memory) {
(bool success, bytes memory result) = addressAt(routeId).delegatecall(
routeData
);
function performAction(
address fromToken,
address toToken,
uint256 amount,
address receiverAddress,
bytes calldata swapExtraData
) external payable override returns (uint256) {
uint256 returnAmount;
if (fromToken != NATIVE_TOKEN_ADDRESS) {
...
{
...
(bool success, bytes memory result) = ONEINCH_AGGREGATOR.call(
swapExtraData //<-- here we do not use the value
);
...
}
} else {
....
(bool success, bytes memory result) = ONEINCH_AGGREGATOR.call{
value: amount //<-- here we use the value
}(swapExtraData);
...
}
...
}
Assume the user sent some ETH, but sent a payload with fromToken != NATIVE_TOKEN_ADDRESS (and the user has already approved the gateway for fromToken). Then, the ether is not used in the transaction and remains stuck in the SocketGateway contract. This is because the function only executes the part of the code that transfers and swaps ERC-20 tokens, but not the part that handles ether.
Now, suppose another user calls the performAction function with fromToken == NATIVE_TOKEN_ADDRESS and provides enough gas to execute the function. Since there is ether stuck in the contract, this user can force the contract to use the stuck ether to execute the swap by sending the exact amount of ether stuck in the contract as the value of the transaction, effectively stealing the funds.
This is why it’s important to ensure that ether is only accepted when it is needed and not left stuck in the contract, as it can be vulnerable to theft in future transactions.
One could be tempted to fix the issue by requiring that the gateway balance always equals 0 at the end of the transaction. However, this is not a good idea, as anyone could cause a Denial of Service in the gateway by sending a tiny amount of ETH.
One might also be tempted to fix this issue by requiring that msg.value == 0 iff fromToken != NATIVE_TOKEN_ADDRESS. However, this also poses a problem, as the gateway might execute multiple routes in a “for” loop. This could lead to reverting valid transactions (when both native and non-native tokens are involved).
The best way to solve this issue might be to compare the balance of the gateway before and after the transaction in all relevant functions. The balance should stay the same otherwise, something wrong happened, and we should revert the transaction. This could be implemented by adding a modifier in SocketGateway, that compares the balance of the gateway before and after the function call. Below is an example to illustrate the idea.
modifier checkGatewayBalance() {
uint256 initialBalance = address(this).balance;
_;
uint256 finalBalance = address(this).balance;
require(initialBalance == finalBalance, "Gateway balance changed during execution");
}
One would also need to introduce a safeExecuteRoute function that calls executeRoute, but adds the modifier. Note that the other gateway functions calling executeRoute in a loop also need to be fixed (such as swapAndMultiBridge…). The executeRoute function could be made internal. However, note that one would also need to introduce an admin-protected function that can perform arbitrary delegatecalls on the different routes, without the balance check (such as the current _executeRoute function) in case some refunds need to be processed manually (cf. issue 6.1)
6.8 No Event Is Emitted When Invoking a Route Through the socketGateway Fallback Function Minor
Resolution
Description
When a route is invoked through executeRoute, or executeRoutes functions, a SocketRouteExecuted event is emitted. However, a route can also be executed by invoking the fallback function of the socketGateway. And in that case, no event is emitted. This might impact off-chain systems that rely on those events.
Recommendation
Consider also emitting a SocketRouteExecuted event in case the route is invoked through the fallback function
6.9 Unused Error Codes. Minor
Resolution
Description
SocketErrors.sol has errors that are defined but are not used:
error RouteAlreadyExist();error ContractContainsNoCode();error ControllerAlreadyExist();error ControllerAddressIsZero();
It seems that they were created as errors that may have been expected to occur during the early stages of development, but the resulting architecture doesn’t seem to have a place for them currently.
Examples
src/errors/SocketErrors.sol:L12-L19
error RouteAlreadyExist();
error SwapFailed();
error UnsupportedInterfaceId();
error ContractContainsNoCode();
error InvalidCelerRefund();
error CelerAlreadyRefunded();
error ControllerAlreadyExist();
error ControllerAddressIsZero();
Recommendation
Consider revisiting these errors and identifying whether they need to remain or can be removed.
6.10 Inaccurate Interface. Minor
Resolution
Description
ISocketGateway implies a bridge(uint32 routeId, bytes memory data) function, but there is no socket contract with a function like that, including the SocketGateway contract.
Examples
src/interfaces/ISocketGateway.sol:L32-L35
function bridge(
uint32 routeId,
bytes memory data
) external payable returns (bytes memory);
Recommendation
Adjust the interface.
6.11 Validate Array Length Matching Before Execution to Avoid Reverts Minor
Resolution
Description
The Socket system not only aggregates different solutions via its routes and controllers but also allows to batch calls between them into one transaction. For example, a user may call swaps between several DEXs and then perform a bridge transfer.
As a result, the SocketGateway contract has many functions that accept multiple arrays that contain the necessary data for execution in their respective routes. However, these arrays need to be of the same length because individual elements in the arrays are intended to be matched at the same indices:
src/SocketGateway.sol:L196-L218
function executeRoutes(
uint32[] calldata routeIds,
bytes[] calldata dataItems,
bytes[] calldata eventDataItems
) external payable {
uint256 routeIdslength = routeIds.length;
for (uint256 index = 0; index < routeIdslength; ) {
(bool success, bytes memory result) = addressAt(routeIds[index])
.delegatecall(dataItems[index]);
if (!success) {
assembly {
revert(add(result, 32), mload(result))
}
}
emit SocketRouteExecuted(routeIds[index], eventDataItems[index]);
unchecked {
++index;
}
}
}
Note that in the above example function, all 3 different calldata arrays routeIds, dataItems, and eventDataItems were utilizing the same index to retrieve the correct element. A common practice in such cases is to confirm that the sizes of the arrays match before continuing with the execution of the rest of the transaction to avoid costly reverts that could happen due to “Index out of bounds” error.
Due to the aggregating and batching nature of the Socket system that may have its users rely on 3rd party offchain APIs to construct these array payloads, such as from APIs of the systems that Socket is integrating, a mishap in just any one of them could cause this issue.
Recommendation
Implement a check on the array lengths so they match.
6.12 Destroyed Routes Eth Balances Will Be Left Locked in SocketDeployFactory Minor
Resolution
Description
SocketDeployFactory.destroy calls the killme function which in turn self-destructs the route and sends back any eth to the factory contract. However, these funds can not be claimed from the SocketDeployFactory contract.
Examples
src/deployFactory/SocketDeployFactory.sol:L170
function destroy(uint256 routeId) external onlyDisabler {
Recommendation
Make sure that these funds can be claimed.
6.13 Possible Double Spends of msg.value in Code Paths That Include More Than One Delegatecall Minor
Resolution
The client team has responded with the following note:
Adding the recommended CI/CD task to verify that future routes are delegate safe.
Description
The usage of msg.value multiple times in the context of a single transaction is dangerous and may lead to loss of funds as previously seen (in a different variation) in the Opyn hack. We were not able to find any concrete instance of the described issue, however, we do see how this pitfall may become an issue in future delegatee contracts.
Examples
Every code path that includes multiple delegatecalls, including:
SocketGateway.swapAndMultiBridge- the
swapAndBridgefunction in all the different route contracts.
Recommendation
Consider implementing this recommendation.
Appendix 1 - Files in Scope
This audit covered the following files:
| File | SHA-1 Hash |
|---|---|
| /src/interfaces/ISocketRequest.sol | d8b481542fa4d3c5ef1cd1a4b49cb4904a05f58e |
| /src/interfaces/ISocketGateway.sol | d9dfe384c55769f234d34915a53f02c74fa7a8e7 |
| /src/interfaces/ISocketRoute.sol | 30b5463db28cbf31a2ed05df722213d80405d6fa |
| /src/interfaces/ISocketController.sol | ebf9995a6f012633ee70996c8a8041c68b2c504a |
| /src/interfaces/ISocketBridgeBase.sol | a2c64914a161f5a52a06f4289eaf6152bfa5e53d |
| /src/controllers/BaseController.sol | 37f610b3436923af895eb131921becc0d656fd56 |
| /src/controllers/RefuelSwapAndBridgeController.sol | 46686c77fa0ff59c5889c92a51c59207888efda1 |
| /src/controllers/FeesTakerController.sol | 02c6a81155bce74cf0d6b09670584792119435ff |
| /src/errors/SocketErrors.sol | 7f530681f134dbcdfd83e18e22b1afe71b7dede1 |
| /src/SocketGateway.sol | f571078fd18a0092736a99271977747512ebb66a |
| /src/static/RouteIdentifiers.sol | 8412534e55dac721ffc7aa438ae3d290bfbed4aa |
| /src/bridges/BridgeImplBase.sol | 4ea3b4268d5c7d9be824a7e9fe73ed51b95c49c1 |
| /src/bridges/cbridge/CelerStorageWrapper.sol | 903b8e56d10ce34fa4b5d6a37ca070b19f95912a |
| /src/bridges/cbridge/CelerImpl.sol | 66da779c3f383e136b77fe8877d2e7b5dc24a440 |
| /src/bridges/cbridge/interfaces/ICelerStorageWrapper.sol | cb54f473c4460969a9766b7d8c22aa7df186ece5 |
| /src/bridges/cbridge/interfaces/cbridge.sol | 60b4a653355544fcf4f65567e284dc5dc2c509a6 |
| /src/bridges/stargate/l1/Stargate.sol | 88a964eae3e0c8d67596280af5a03b76db116684 |
| /src/bridges/stargate/interfaces/stargate.sol | e02de8f547cbcee020a680a7a60bc1eb9f817065 |
| /src/bridges/stargate/l2/Stargate.sol | 7a33d727859ba83df9af5f185b5450b51b4e4c14 |
| /src/utils/Ownable.sol | 9f7f3a88bf1593e32cdb513e6151d6a5cf3f36e9 |
| /src/swap/SwapImplBase.sol | 2555cdac41c3d4ea5d56168365e2b6544ec06288 |
| /src/swap/oneinch/OneInchImpl.sol | 28f646473dfe0c31ebee7869f1f69fd88ed07088 |
| /src/deployFactory/DisabledSocketRoute.sol | 64a48bc0c2d7afb83d78942147933d12fd7041a3 |
| /src/libraries/LibBytes.sol | ee29b785d7e73b73cd7637a71717223b39fccd03 |
| /src/deployFactory/SocketDeployFactory.sol | 5fcd6dd59e683dd6a7429a970d3cf059997f1fe7 |
| /src/libraries/LibUtil.sol | 305421b34cc1adf07f16f6d8a3bc03f330357f9a |
| /src/libraries/Pb.sol | 7af0ce1286b28f29e1ab58e60d8c169e74b5dbca |
Appendix 2 - Disclosure
ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.
The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.
PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of code and only the code we note as being within the scope of our review within this report. Any Solidity code itself presents unique and unquantifiable risks as the Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond specified code that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. In some instances, we may perform penetration testing or infrastructure assessments depending on the scope of the particular engagement.
CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.
LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.
TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.