Decent - windhustler's results

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L96

Vulnerability details

Impact

While relaying payloads through DecentEthRouter the gas for relayer to submit the transaction on the receiving chain is hardcoded to 100_000. This was hardcoded with EVM compatible chains in mind and works fine expect for Arbitrum that has a totally different gas model than Ethereum. Differences are summarized in the following article: https://docs.arbitrum.io/devs-how-tos/how-to-estimate-gas. As an example even a simple approve transaction requires more than 100_000 gas on Arbitrum: https://arbiscan.io/tx/0x04f97a39c6d03029518d1953226b343555ea585d2064a24a767155a548b0665e.

As this is hardcoded it will result with messages being sent to Arbitrum to always revert at the beginning of the lzReceive, and will result in blocked pathway between all chains and Arbitrum.

The impact is that the pathway between all chains and Arbitrum will be blocked and no messages can be sent to Arbitrum.

Proof of Concept

DecentEthRouter hardcodes 100_00 gas for the relayer to deliver the message to remote chain. If this is invoked from some chain to Arbitrum the following happens:

1. The transaction is submitted to Arbitrum with 100_000 gas as gasLimit.
2. It enters lzReceive and reverts.
3. This results in storedPayload and blocking the pathway due to nonce ordering.

Tools Used

• Manual review

Recommended Mitigation Steps

Don't hardcode gas costs for each chain but implement a different gas model for Arbitrum. A reference point for calculating equivalent gas costs for Arbitrum is the following: https://docs.arbitrum.io/devs-how-tos/how-to-estimate-gas.

Assessed type

Other

Lines of code

https://github.com/decentxyz/decent-bridge/blob/7f90fd4489551b69c20d11eeecb17a3f564afb18/src/DecentEthRouter.sol#L185-L193

Vulnerability details

Impact

Due to variable cost of saving failed messages inside the failedMessages storage variable, it is possible to block the LayerZero pathway by sending a big payload with the default amount of gas.

The impact is that the LayerZero pathway gets blocked and no other messages can be sent through it.

Proof of Concept

Let's examine the DecentEthRouter:bridgeWithPayload() function:

1. It uses the sendAndCall function to send the payload to the receiving chain.
2. The adapterParams and the corresponding gasLimit to deliver the message are set to 100_000 plus any additional gas for the external call inside the onOFTReceived function.
3. If the onOFTReceived function fails, the messages is being saved into the failedMessages storage variable.
4. A malicious user can force the onOFTReceived function to fail, just to provoke saving the message. I'll leave the test to showcase the cost of saving the message in the failedMessages storage variable and emitting an event.
5. As the cost for bigger payloads is around 131k, and all the extra gas is used in the onOFTReceived function, the uint256 GAS_FOR_RELAY = 100000; is not enough to execute all the logic.
6. If the transaction fails inside the _sendAndCallAck function it will result in storedPayload inside the Endpoint.sol and block the LayerZero pathway.

Gas costs of storing big paylaod inside the failedMessages storage variable and emitting an event:

// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

contract FailedMessagesTest is Test {

    mapping(uint16 => mapping(bytes => mapping(uint64 => bytes32))) public failedMessages;

    event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);

    function setUp() public {}

    function testFMessagesGas() public {
        uint16 srcChainid = 1;
        bytes memory srcAddress = abi.encode(makeAddr("Alice"));
        uint64 nonce = 10;
        bytes memory payload = getDummyPayload(9999); // max payload size someone can send is 9999 bytes
        bytes memory reason = getDummyPayload(2);

        uint256 gasLeft = gasleft();
        _storeFailedMessage(srcChainid, srcAddress, nonce, payload, reason);
        emit log_named_uint("gas used", gasLeft - gasleft());
    }


    function _storeFailedMessage(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload,
        bytes memory _reason
    ) internal virtual {
        failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
        emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
    }

    function getDummyPayload(uint256 payloadSize) internal pure returns (bytes memory) {
        bytes memory payload = new bytes(payloadSize);
        for (uint256 i = 0; i < payloadSize; i++) {
            payload[i] = bytes1(uint8(65 + i));
        }
        return payload;
    }
}

Tools Used

• Manual review

Recommended Mitigation Steps

Estimate the maximum gas cost for big payloads and set the GAS_FOR_RELAY considering that value. The exact fix for this issue will also depend on how you decide to fix issues I mentioned in my other reports.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/bridge_adapters/StargateBridgeAdapter.sol#L183

Vulnerability details

Impact

While sending funds through StargateBridgeAdapter, the user passes the swap data as a parameter. The flow is stargate sending tokens on the receiving chain into the StargateBridgeAdapter and executing sgReceive.

The issue here is that sgReceive will fail if the swap data gets outdated, but this is not going to make the whole transaction revert.

Stargate will still send the tokens to the StargateBridgeAdapter, and if the sgReceive fails, the swap will be cached for later execution. See StargateComposer logic here: https://stargateprotocol.gitbook.io/stargate/stargate-composability/stargatecomposer.sol#sgreceive.

Now, tokens will be left sitting in the StargateBridgeAdapter contract, and since the user can only retry the transaction with the same swap data, the tokens will be stuck forever.

The impact is loss of transferred tokens for the user.

Proof of Concept

Consider the following flow:

1. User invokes StargateBridgeAdapter:bridge() function as part of the whole flow of calling UTB:bridgeAndExecute() function.
2. He is passing the swapData parameters to the remote chain.
3. StargateComposer first transfers the tokens to the StargateBridgeAdapter contract.
4. After that the StargateBridgeAdapter:sgReceive() function is executed.
5. It then calls UTB:receiveFromBridge that tries to execute the swap.
6. If the swap fails, which can frequently occur due to the fact that the swap data is outdated, most commonly minimum amount out gets outdated.
7. The whole payload gets stored inside the StargateComposer contract, and the tokens are left in the StargateBridgeAdapter contract.
8. As the user can only retry the transaction with the same payload, the tokens will be stuck forever.
9. An example of StargateComposer contract can be seen on: https://stargateprotocol.gitbook.io/stargate/developers/contract-addresses/mainnet#ethereum

Note: The application needs to use the StargateComposer contract as opposed to StargateRouter because it is transferring funds with payload, i.e. it executes sgReceive on the receiving chain. You can only use StargateRouter directly if you're sending funds empty payload. More on it: https://stargateprotocol.gitbook.io/stargate/stargate-composability

Tools Used

• Manual review

Recommended Mitigation Steps

Wrap the whole StargateBridgeAdapter:receiveFromBridge() call into a try/catch and if it reverts send the transferred tokens back to the user.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/UTB.sol#L186

Vulnerability details

Impact

The UTB:bridgeAndExecute() assumes that stargate transfers the amount of tokens specified in the minAmountOut parameter of the swap function. In reality, it can transfer more than that and this difference is lost for the user.

As this is a direct loss of funds for the user, I rate this as a high severity issue.

Proof of Concept

The flow while using the UTB:bridgeAndExecute() function is as follows:

1. First a swap is executed, i.e. UTB:performSwap function.
2. After that the amountIn is set for the newPostSwapParams
3. If the adapter is StargatBridgeAdapter, this amount is calculated using the actual amount received after the swap and applying the fee deduction.
4. The fee is hardcoded inside the StargatBridgeAdapter and is set to 6 BPS.
5. The issue here that stargate swap function works so that you can pass the minimum amount out but the actual tokens transferred can be higher than that.
6. The minimum amount is just there to protect the user from getting less than the minimum amount.
7. This can be observed by looking at the Stargate Pool contract code: https://github.com/stargate-protocol/stargate/blob/main/contracts/Pool.sol#L187

Let's take an example of 5 tokens received after the swap.

1. After applying the fee deduction we get 4.997 tokens which is the minimum amount the user expects to receive.
2. But in reality the user receives 4.999 tokens on the destination chain.
3. The difference is lost for the user.

As only the set amountIn in the newPostSwapParams is approved and sent to utb, the leftover tokens are left sitting in the StargateBridgeAdapter contract.

Tools Used

• Manual review

Recommended Mitigation Steps

When the sgReceive is invoked on the receiving chain check the actual amount of received tokens and used that for swapping. Don't leave any tokens hanging in the StargateBridgeAdapter contract.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/011f62059f3a0b1f3577c8ccd1140f0cf3e7bb29/src/bridge_adapters/StargateBridgeAdapter.sol#L23

Vulnerability details

Impact

Hardcoding the stargate fee to calculate the minimum amount out is problematic due to 2 reasons:

• Stargate can update the fee library at any time which will break the contract
• Even if the fee library is not updated, the fee can be higher than the hardcoded value and the transaction will revert

As this is the main functionality of the system and hardcoding fee can cause the system to break, but there is no risk of losing funds, I would classify this as a medium risk.

Proof of Concept

Stargate's swap interface lets the caller specify the minimum amount of tokens he wants to receive on another chain: https://github.com/stargate-protocol/stargate/blob/main/contracts/Router.sol#L107-L133. Each pool has a variable fee calculated by the fee library: https://github.com/stargate-protocol/stargate/blob/main/contracts/Pool.sol#L164-L212 The fee library can be freely set at any time: https://github.com/stargate-protocol/stargate/blob/main/contracts/Pool.sol#L413. The StargateBridgeAdapter might work with the hardcoded fee value for the exisiting fee library, but it will break if the fee library is updated to have different logic.

As a consequence hardcoding minAmountOut doesn't work as the fee can be higher than the hardcoded value and the transaction will revert. if we look at the currently deployed FeeLibrary it is version 7: https://stargateprotocol.gitbook.io/stargate/developers/contract-addresses/mainnet

Tools Used

• Manual review

Recommended Mitigation Steps

Allow the user to specify the minimum amount out and don't hardcode it.

Assessed type

Other