Core & Bridge - YieldFi

The send functions in the BridgeLR and BridgeMB contracts, as well as the quote function in the Bridge contract, allow users to specify the _receiverGas parameter, which determines how much gas should be used when calling lzReceive on the destination endpoint.

The current condition require(_receiverGas > 0, "!gas"); is not sufficient to prevent users from providing a very small value for _receiverGas. This can lead to a situation where the safeTransferFrom function is executed on the source chain, but the message is not processed on the destination chain due to insufficient gas, resulting in a loss of funds.

- contracts/bridge/BridgeMB.sol

    function send(
        address _srcToken,
        uint32 _dstId,
        address _receiver,
        uint256 _amount,
        uint128 _receiverGas
    ) external payable notBlacklisted(msg.sender) notBlacklisted(_receiver) notPaused {
        require(_amount > 0, "!amount");
        require(tokens[_srcToken][_dstId] != address(0), "!desToken");
        require(IERC20(_srcToken).balanceOf(msg.sender) >= _amount, "!balance");
        require(_receiver != address(0), "!receiver");
        require(_receiverGas > 0, "!gas");
      
        IMinter(_srcToken).burn(msg.sender, _amount);

        bytes memory _options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(_receiverGas, 0);

        bytes memory _encodedMessage = abi.encode(
            _dstId,
            _receiver,
            tokens[_srcToken][_dstId],
            _amount,
            Constants.BRIDGE_SEND_HASH
        );

        _lzSend(
            _dstId,
            _encodedMessage,
            _options,
            // Fee in native gas and ZRO token.
            MessagingFee(msg.value, 0),
            // Refund address in case of failed source message.
            payable(msg.sender)
        );
    }

- contracts/bridge/BridgeLR.sol

    function send(
        address _srcToken,
        uint32 _dstId,
        address _receiver,
        uint256 _amount,
        uint128 _receiverGas
    ) external payable notBlacklisted(msg.sender) notBlacklisted(_receiver) notPaused {
        require(_amount > 0, "!amount");
        require(lockboxes[_srcToken] != address(0), "!token !lockbox");
        require(IERC20(_srcToken).balanceOf(msg.sender) >= _amount, "!balance");
        require(_receiver != address(0), "!receiver");
        require(tokens[_srcToken][_dstId] != address(0), "!destination");
        require(_receiverGas > 0, "!gas");

        bytes memory _options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(_receiverGas, 0);
        bytes memory _encodedMessage = abi.encode(
            _dstId,
            _receiver,
            tokens[_srcToken][_dstId],
            _amount,
            Constants.BRIDGE_SEND_HASH
        );

        // Lock the tokens
        IERC20(_srcToken).safeTransferFrom(msg.sender, lockboxes[_srcToken], _amount);
        emit ILockBox.Lock(msg.sender, _srcToken, msg.sender, _amount);

        // Sends the message to the destination endpoint
        _lzSend(
            _dstId,
            _encodedMessage,
            _options,
            // Fee in native gas and ZRO token.
            MessagingFee(msg.value, 0),
            // Refund address in case of failed source message.
            payable(msg.sender)
        );
    }

- contracts/bridge/Bridge.sol

    function quote(
        uint32 _dstEid,
        bytes memory _payload,
        bool _payInLzToken,
        uint128 _receiverGas
    ) public view returns (MessagingFee memory fee) {
        bytes memory _options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(_receiverGas, 0);
        fee = _quote(_dstEid, _payload, _options, _payInLzToken);
    }

In order to reproduce this vulnerability, call the send function passing as function parameter a small value for _receiverGas, such as 1 wei. The transaction will not revert in source chain, effectively burning user's tokens. However, it will not be processed in the destination chain, considering insufficient gas provided.

• PoC Code:

    function testSendWithLowReceiverGas() public {
        vm.startPrank(user);

        // Approve the bridge to spend user's tokens
        token.approve(address(bridgeSrc), 100 * 1e6); // User wants to send 100 tokens

        // Set low _receiverGas
        uint128 receiverGas = 1;

        // Capture the balance before sending
        uint256 userBalanceBefore = token.balanceOf(user);

        // Execute the send function
        bridgeSrc.send{value: 0}(
            address(token),    // _srcToken
            100,               // _dstId
            user,              // _receiver
            100 * 1e6,         // _amount
            receiverGas        // _receiverGas (set to 1)
        );

        // Capture the balance after sending
        uint256 userBalanceAfter = token.balanceOf(user);

        vm.stopPrank();

        // Assert that the user's tokens have been burned
        assertEq(userBalanceAfter, userBalanceBefore - 100 * 1e6, "Tokens were not burned");
    }

• Logs:

It is recommended to enforce a minimum _receiverGas value in order to ensure transactions will be processed in the destination chain.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

The withdrawRequest function is permissionless, allowing anyone to request a withdrawal on behalf of an owner to a receiver, as long as the msg.sender has sufficient allowance. This can lead to a Denial of Service (DoS) condition, even if the msg.sender has a relatively small amount of tokens.

Each withdrawal request resets the cooling period and accumulates the withdrawal amount, as indicated in the following code snippet:

- contracts/core/SToken.sol

withdrawals[receiver].coolingPeriod = block.timestamp + COOLING_PERIOD;

This means that even a minimal request of 1 wei, performed thousands of times (still less than 1 token, i.e., 1e18), would keep resetting the cooling period.

An attacker can continuously reset the cooling period by making new withdrawal requests, preventing users from claiming their funds.

To reproduce this issue, deposit as u1 and call multiple times the withdrawRequest as user (with sufficient allowance). This will effectively increase the cooldown period for u1, leading to a denial of service.

• PoC Code:

    function testCoolingPeriodNotResetOnMultipleRequests() public {
        // User deposits and requests withdrawal
        vm.startPrank(u1);
        usdt.approve(address(stoken), 1_000_000e6);
        stoken.approve(address(user), 1_000_000 * 1e18);
        stoken.deposit(1_000_000e6, u1);
        stoken.withdrawRequest(500_000e18, u1, u1);
        (uint256 cooldownBefore, ) = stoken.withdrawals(u1);
        vm.stopPrank();

        vm.startPrank(user);
        
        for (uint8 i = 0; i < type(uint8).max - 1; i++) {
            // Warp 10 seconds ahead
            vm.warp(block.timestamp + 10);
            stoken.withdrawRequest(1, u1, u1);
        }
        
        vm.stopPrank();

        // Check that the cooling period is greater
        (uint256 cooldownAfter, ) = stoken.withdrawals(u1);

        assertGt(cooldownAfter, cooldownBefore);
    }

• Logs:

It is recommended to review the cooldown mechanism, and also the permissionless property of the claim function, in order to prevent malicious users to DoS legitimate transactions (claims).

SOLVED: The YieldFi team has solved this issue by implementing a NFT receipt mechanism. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

The YTokenL2.sol contract calls the getPrice function from the oracle but does not validate the returned price against zero. This lack of validation can lead to a division by zero error in both the deposit and withdraw functions. Specifically, if the price returned by the oracle is zero, the calculation of shares will result in a division by zero.

- contracts/core/YTokenL2.sol

        uint256 _price = IOracle(oracle).getPrice(address(this));
        shares = (sAmount * Constants.PINT) / _price ;
        _mint(receiver, shares);

This can lead to a scenario of consistent reverts because of [Revert] panic: division or modulo by zero (0x12).

In order to reproduce this vulnerability, it is needed to mock an Oracle answer of 0 (zero). Then, call the withdraw or deposit functions in the affected contract. The transaction will revert (panic), because of division by zero.

• PoC Code:

    function testDepositWithZeroPrice() public {
        vm.startPrank(user);

        // Approve YTokenL2 to spend user's USDT
        usdt.approve(address(yTokenL2), 100 * 1e6);

        // Attempt to deposit USDT
        uint256 depositAmount = 100 * 1e6; // 100 USDT

        // Capture balances before deposit
        uint256 userUSDTBalanceBefore = usdt.balanceOf(user);
        uint256 sTokenUSDTBalanceBefore = usdt.balanceOf(address(sToken));

        // Expect the transaction to revert due to division by zero
        vm.expectRevert();

        // Call deposit function
        yTokenL2.deposit(address(usdt), depositAmount, user);

        vm.stopPrank();

        // Check balances after failed deposit
        uint256 userUSDTBalanceAfter = usdt.balanceOf(user);
        uint256 sTokenUSDTBalanceAfter = usdt.balanceOf(address(sToken));

        // Assert that user's USDT balance decreased
        assertEq(
            userUSDTBalanceAfter,
            userUSDTBalanceBefore - depositAmount,
            "User's USDT balance should have decreased"
        );

        // Assert that sToken received the USDT
        assertEq(
            sTokenUSDTBalanceAfter,
            sTokenUSDTBalanceBefore + depositAmount,
            "sToken should have received the USDT"
        );

        // Assert that user's YTokenL2 balance did not increase
        uint256 userYTokenBalance = yTokenL2.balanceOf(user);
        assertEq(userYTokenBalance, 0, "User's YTokenL2 balance should be zero");
    }

• Logs:

To resolve this issue, add a check to ensure that the price returned by the oracle is not zero before performing the division. If the price is zero, the transaction should be reverted.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

Several contracts within the scope of this audit use modifiers to restrict certain functions to administrators, owners, and other high-privileged parties. These functions involve direct interactions with token amounts, such as transferring or managing funds.

This design introduces a significant centralization risk, as malicious administrators can potentially drain the protocol. Additionally, if the private keys of these privileged accounts are leaked, it could lead to permanent loss of funds.

Use multi-signature wallets for administrative functions. This ensures that multiple parties must approve any administrative action, reducing the risk of a single malicious actor or leaked private key.

SOLVED: The YieldFi team has solved this issue as recommended, using multi-signature wallets.

In the Yield contract, the distributeYield function is checking if the provided payload.epoch is greater than the last recorded epoch for the receiver. This allows epochs to be skipped.

Because of missing input validation, if the rewarder submits a payload with a significantly larger epoch value, future legitimate rewards will be unclaimable because the epoch has been artificially advanced.

- contracts/core/Yield.sol

    function distributeYield(bytes calldata data, bytes memory proofSignature) external notPaused onlyRewarder {
        bytes32 proofHash = keccak256(data);
        require(!trxns[proofHash], "!new txcn");
        validate(proofHash, proofSignature);

        RewardPayload memory payload = Codec.decodeReward(data);

        require(payload.epoch > epoch[payload.receiver], "!epoch");

        trxns[proofHash] = true;
        epoch[payload.receiver] = payload.epoch;
        
        if (payload.rewardType == 1) { // profit 
             _distributeYield(payload.receiver, payload.amount, true);
            profit[payload.receiver] += payload.amount;
        } else { // loss
            _distributeYield(payload.receiver, payload.amount, false);
            loss[payload.receiver] += payload.amount;
        }

        emit DistributeYield(IERC4626(payload.receiver).asset(), payload.receiver, payload.amount, payload.rewardType == 1);
    }

It is recommended to add a more granular control, enforcing sequential epochs. This can be achieved by modifying the require statement as follows:

require(payload.epoch == epoch[payload.receiver] + 1, "!epoch");

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

The LockBox contract within the scope of this audit uses the standard ERC20 function approve, which may not behave as expected in all scenarios.

To ensure robust and secure interactions with ERC20 tokens, it is recommended to use OpenZeppelin's SafeERC20 library. This library provides wrapper functions that include additional checks and safeguards, ensuring that token transfers and other operations are performed safely and correctly.

It is recommended to use the SafeERC20 method forceApprove, instead of unsafe ERC20 approve.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

During the analysis of the SToken contract, it was identified that the setCoolingPeriod function allows setting the cooling period to very low values, which might undermine its purpose.

- contracts/core/SToken.sol

    function setCoolingPeriod(uint256 period) external onlyAdmin {
        require(period > 0 && period <= Constants.MAX_COOLDOWN_PERIOD, "!period");
        COOLING_PERIOD = period;
    }

It is recommended to define a state variable for a reasonable minimum cooling period.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

Some contracts in-scope are utilizing the ReentrancyGuard from OpenZeppelin for reentrancy locks (mutex), which is best practice. However, the nonReentrant modifier should be placed before all other modifiers, to effectively prevent reentrancy on the function's modifiers.

- contracts/core/LockBox.sol

function unlock(address token, address to, uint256 amount) external notPaused nonReentrant onlyBridge {

- contracts/core/SToken.sol

) public notPaused nonReentrant returns(uint256 sAmount) {

- contracts/core/SToken.sol

) public notPaused nonReentrant {

- contracts/core/SToken.sol

) public notPaused nonReentrant {

- contracts/core/STokenL2.sol

) public notPaused nonReentrant returns(uint256 sAmount) {

- contracts/core/YToken.sol

function transferInRewards(uint256 amount, bool profit) external notPaused nonReentrant onlyYield {

- contracts/core/YToken.sol

) public notPaused nonReentrant returns(uint256 shares) {

It is recommended to place the nonReentrant modifier before all other modifiers.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from Ethereum mainnet, like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

ACKNOWLEDGED: The YieldFi team has acknowledged this finding.

The Access and YToken contracts contain modifiers that are invoked only once within the contracts. While modifiers are useful for reusing code and improving readability, their use in this context is inefficient and can make the code harder to understand.

Integrating the modifier's logic directly into the function's body can improve performance and maintainability.

- contracts/administrator/Access.sol

modifier onlyBridge() {

- contracts/core/YToken.sol

modifier onlyYield() {

It is recommended to integrate the modifier's logic directly into the function's body.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is 9e2e3842a08a08f0d1226300ef7eea3f8e68703d.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, for example:

error ConditionNotMet();

ACKNOWLEDGED: The YieldFi team has acknowledged this finding.

The contracts within the scope of this audit do not emit events after performing important operations, such as transferring funds, updating state variables, or executing critical functions.

Event emission is crucial for transparency and auditability, as it allows external observers and off-chain services to track and verify the contract's state changes. The absence of event emissions can hinder monitoring, debugging, and integration with other systems, potentially leading to reduced trust and security in the contract's operations.

The following functions are not emitting relevant events:

• Bridge::setAdministrator

• Access:setAdministrator

• MPC::setMPCs

• YToken::rescue

It is recommended to emit events after performing important operations. This will enhance transparency and allow external observers to track and verify the contract's state of changes.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is d051ac1c4a9e6ac736f6586a21ec582b4144b184.

Multiple contracts within the scope of this audit perform redundant balance checks immediately before transferring funds using SafeERC20 methods.

These balance checks are unnecessary because the SafeERC20 methods already include internal checks to ensure that the transfer does not exceed the available balance. These redundant checks consume additional gas and can be safely removed to optimize the contract's performance.

- contracts/bridge/BridgeLR.sol

        require(IERC20(_srcToken).balanceOf(msg.sender) >= _amount, "!balance");

- contracts/bridge/BridgeMB.sol

        require(IERC20(_srcToken).balanceOf(msg.sender) >= _amount, "!balance");

- contracts/core/SToken.sol

        require(IERC20(usdt).balanceOf(msg.sender) >= amount, "!amount");

- contracts/core/STokenL2.sol

        require(IERC20(usdt).balanceOf(msg.sender) >= amount, "!amount");

- contracts/core/YToken.sol

        require(IERC20(usdt).balanceOf(msg.sender) >= amount, "!amount");

It is recommended to remove the redundant balance checks before calling the SafeERC20 methods.

The SafeERC20 methods already handle balance checks internally, ensuring that the transfer does not exceed the available balance.

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is d051ac1c4a9e6ac736f6586a21ec582b4144b184.

Some of the smart-contracts in-scope contain loops that perform increment operations using checked arithmetic and the post-increment operator (i++).

In Solidity versions 0.8.0 and above, arithmetic operations are checked for over/underflows by default, which introduces additional gas costs. Additionally, using the post-increment operator is less gas-efficient, compared to the pre-increment operator (++i).

For enhanced gas consumption, consider implementing loops following the example:

for (uint256 i; i < n;) {
   
    /// Loop Logic

    unchecked {
        ++i
    }

}

SOLVED: The YieldFi team has solved this issue as recommended. The commit hash for reference is d051ac1c4a9e6ac736f6586a21ec582b4144b184.