Sofa Protocol
Findings & Analysis Report

2024-06-06

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 Pro League Audit is an event where elite tier Code4rena contributors, commonly referred to as wardens, reviews, audits and analyzes smart contract logic in exchange for a bounty provided by sponsoring projects.

During the Pro League audit outlined in this document, C4 conducted an analysis of the Sofa Protocol smart contract system written in Solidity. The audit took place between May 13 - May 28, 2024.

Wardens

3 Wardens contributed to Sofa Protocol:

  1. carrotsmuggler
  2. peakbolt
  3. elhaj

Final report assembled by bytes032 and thebrittfactor.

Summary

The C4 Pro League analysis yielded an aggregated total of 9 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 6 received a risk rating in the category of MEDIUM severity.

Additionally, C4 Pro League analysis included 3 findings with a risk rating of LOW severity or non-critical and 1 Informational finding.

Scope

The source code was delivered to Code4rena in a Git repository at https://github.com/sofa-org/sofa-protocol. The audit was performed against commit 4fe95404fbb87023374af424cd35ebe4bcce747b.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (3)

[H-01] Aave Vaults are vulnerable to share inflation attacks

Lines of Code

Description

In the Aave vaults, SHARE_MULTIPLIER is used to prevent share inflation attack by increasing share precision. This is to prevent attacks that cause an user’s share to be rounded down to zero or a small number, thus receiving incorrect share.

However, the _mint() actually reduces the share precision again with a division by SHARE_MULTIPLIER, causing user to be minted incorrect shares amount when there is an share inflation attack.

        _mint(_msgSender(), productId, aTokenShare / SHARE_MULTIPLIER, "");
        _mint(params.maker, makerProductId, aTokenShare / SHARE_MULTIPLIER, "");

Recommendation

Remove the division by SHARE_MULTIPLIER so that the minted share amount are in SHARE_MULTIPLIER precision. Note that this will also require changes to other share computation (e.g. in burn(), harvest()) that had assumed collateral precision for shares amount. And decimals() should reflect the increased precision as well.

Sofa Protocol

Fixed in commit #7aab.

Code4rena Pro League

Verified, the share precision is now correctly increased based on SHARE_MULTIPLIER during mint and other share computations.

[H-02] Incorrect Expiry Used in getHlPrices Function When Burning A Product ID Allow Double Withdrawal Exploit in DNT Vaults

Lines of Code

Description

  • The getHlPrices function may include prices after the intended expiry time when the burn() function is called in DNTVault contract after the expiry date (more than 1 day atleast). This can lead to incorrect settlement calculations and allow an attacker to double withdraw his bet.

  • The expiry passed to getHlPrices() might be later than the product’s actual expiry, that’s because we always pass latestExpiry , which is basically current time rounded to getMakerPayoff and getMinterPayoff functions:

        // some code ...
 >>   uint256 latestExpiry = (block.timestamp - 28800) / 86400 * 86400 + 28800;

    if (isMaker == 1) {
 >>     payoff = getMakerPayoff(latestTerm, latestExpiry, anchorPrices, amount);
    } else {

 >>     (payoff, settlementFee) = getMinterPayoff(latestTerm, latestExpiry, anchorPrices, amount);

    }
  • Note that the expiry passed to getHlPrices() might be later than the product’s actual expiry, causing the function to iterate over and include prices that should not be considered and exclude some that should be included. This results in settling the payoff incorrectly, as prices beyond the product’s expiry can skew the high/low price determination.
  >>   payoff = STRATEGY.getMakerPayoff(anchorPrices, ORACLE.getHlPrices(term, expiry), amount);
  • Since the minter and the maker call this function separately, they might use different latestExpiry values if they burn their tokens at different times. This discrepancy can lead to inconsistent settlement values for the two parties.

Attack Example:

  • An attacker could take advantage of that by opening a position with themselves and monitoring the prices after the expiry. When prices are favorable to the minter, the attacker burns the minter’s token id.
  • Conversely, when prices favor the maker, the attacker closes the maker’s position. This allows the attacker to double withdraw their bet, effectively draining the protocol with a large position.

Note: This issue is present in all DNT Vaults.

Recommendation

For DNT vaults, always ensure that the time used for settlement is equal to or less than the product’s real expiry. Implement checks to prevent using a later expiry time:

function _burn(uint256 term, uint256 expiry, uint256[2] memory anchorPrices, uint256 isMaker) internal nonReentrant returns (uint256 payoff) {
       uint256 productId = getProductId(term, expiry, anchorPrices, isMaker);

       (uint256 latestTerm, bool _isBurnable) = isBurnable(term, expiry, anchorPrices);
       require(_isBurnable, "Vault: not burnable");

       // check if settled
-        uint256 latestExpiry = (block.timestamp - 28800) / 86400 * 86400 + 28800;
+        uint256 current = (block.timestamp - 28800) / 86400 * 86400 + 28800;
+        uint256 latestExpiry = current > expiry ? expiry : current

       require(ORACLE.settlePrices(latestExpiry, 1) > 0, "Vault: not settled");
       // more code ...
   }

function _burnBatch(Product[] calldata products) internal nonReentrant returns (uint256 totalPayoff) {
          //some code ..
       for (uint256 i = 0; i < products.length; i++) {
           // check if settled
-            uint256 latestExpiry = (block.timestamp - 28800) / 86400 * 86400 + 28800;
+            Product memory product = products[i];
+            uint256 current = (block.timestamp - 28800) / 86400 * 86400 + 28800;
+            uint256 latestExpiry = current > product.expiry ? product.expiry  : current
+            require(ORACLE.settlePrices(latestExpiry, 1) > 0, "Vault: not settled");

-            Product memory product = products[i];
         }
           // more code ....
       }

Sofa Protocol

Fixed in commit #a83b.

Code4rena Pro League

Verified. The DNT vaults now ensure that settlement calculation will not include prices after product expiry.

[H-03] Signatures from makers can be re-used due to malleability

Lines of Code

Description

Maker signatures used are malleable. The contract uses ecrecover to recover the signer of the signatures, and then stores the hash of v,r,s to denote a used signature.

The issue is that if (v,r,s) is a valid signature, so is (v,r, -s mod n). This is a well known feature of the elliptic curve cryptography. The hash of this manipulated signature is different from the original one, so it allows the same signature to be used twice.

More details about the signature malleability can be found here.

This vulnerability allows maker signatures to be used twice. So makers can be signed up to be exposed to positions twice the size of the position they were anticipating.

Recommendation

Either use the latest openzeppelin ECDSA library, or implement a nonce system for maker signatures to prevent re-use. Openzeppelin ECDSA library already makes sure that the passed s value is only in the lower range.

Sofa Protocol

Fixed in commit #e4d8. It is still considered acceptable regardless of the fix, as the market makers allow users to re-use the signature twice.

Code4rena Pro League

Verified to have adopted OpenZeppelin ECDSA library for signature verification to prevent signature re-use.

Medium Risk Findings (6)

[M-01] Aave Vaults lack support for claiming of reward tokens

Lines of Code

Description

The Aave vaults are designed to supply the collaterals into Aave Pools to receive interest-bearing aTokens. When redeeming the product on expiry using burn()/ethBurn(), the users will be able to receive additional returns from the interests earned in Aave Pools.

However, the Aave Vaults do not support claiming of reward tokens, which are emitted to holders of certain aTokens. For example, aTokens like aUSDC, has 1% APY ARB reward on Arbitrum (see here). That means these reward tokens will be lost as they cannot be claimed.

    function burn(uint256 term, uint256 expiry, uint256[2] calldata anchorPrices, uint256 collateralAtRiskPercentage, uint256 isMaker) external {
        uint256 payoff = _burn(term, expiry, anchorPrices, collateralAtRiskPercentage, isMaker);
        if (payoff > 0) {
            require(POOL.withdraw(address(COLLATERAL), payoff, _msgSender()) > 0, "Vault: withdraw failed");
        }
    }

    function ethBurn(uint256 term, uint256 expiry, uint256[2] calldata anchorPrices, uint256 collateralAtRiskPercentage, uint256 isMaker) external onlyETHVault {
        uint256 payoff = _burn(term, expiry, anchorPrices, collateralAtRiskPercentage, isMaker);
        if (payoff > 0) {
            require(POOL.withdraw(address(COLLATERAL), payoff, address(this)) > 0, "Vault: withdraw failed");
            WETH.withdraw(payoff);
            payable(_msgSender()).transfer(payoff);
        }
    }

Recommendation

For applicable aTokens, implement the reward claim mechanism so that users can get their share amount of the reward before withdrawing from the Aave pool. Reference here.

Sofa Protocol

Acknowledged.

  1. Supporting reward claiming and distribution will significantly increase the complexity > of the contract.
  2. We currently have no plans to support Arbitrum USDC, and there are no rewards for other tokens at this time.

Code4rena Pro League

vAs the supported Aave pools do not have reward tokens currently, the issue is not applicable at this time.

[M-02] Use of ETH transfer() could cause burning of ETH products to fail

Lines of Code

Description

The vault uses transfer() for sending ETH to the caller after determining the payout amount.

           payable(_msgSender()).transfer(payoff);

However, use of transfer() has the risk that it will not work if the gas cost increases/decrease. The assumption that gas cost stays fixed no longer holds true since Istanbul hard fork. see https://swcregistry.io/docs/SWC-134/. When that happens, it will prevent the caller from receiving ETH payout.

In the past transfer() was recommended to prevent re-entrancy attacks (i.e. via receive/fallback) as it limits forwarded gas to 2300. Now the security recommendation to prevent re-entrancy is to ensure Check-Effects-Interactions Pattern or use a Reentrancy guard.

Recommendation

Recommend to change to the following:

        (bool success, ) = _msgSender().call{value: payoff, gas: 100_000}("");
        require(success, "Failed to send ETH");

Sofa Protocol

Fixed in commit #da49.

Code4rena Pro League

Verified to have adopted recommendation to use .call() for ETH transfer.

[M-03] Ineffective swap deadline for swapRCH()

Lines of Code

Description

swapRCH() uses block.timestamp, which renders the swap deadline ineffective. That is because the value of block.timestamp is only determined during execution of the tx, causing the deadline check to always pass.

    function swapRCH(
        address token,
        uint256 minPrice,
        address[] calldata path
    ) external onlyOwner {
        // last element of path should be rch
        require(path.length <= 4, "Collector: path too long");
        require(path[path.length - 1] == rch, "Collector: invalid path");

        uint256 amountIn = IERC20(token).balanceOf(address(this));
        IUniswapV2Router(routerV2).swapExactTokensForTokens(
            amountIn,
            amountIn * minPrice / 1e18,
            path,
            address(this),
            block.timestamp + 10 minutes
        );
    }

Recommendation

This can be fixed by passing in the swap deadline as a parameter in swapRCH() for both routerV2 and routerV3 to ensure an absolute deadline.

Sofa Protocol

Fixed in commit #d874.

Code4rena Pro League

Verified to have adopted recommendation, which is to make deadline an user input parameter.

[M-04] Failure to set settlePrices[] will prevent redemption of product

Lines of Code

Description

Both SpotOracle and HlOracle are responsible for recording the settlePrices[] via settle().

    function settle() public {
        uint256 expiry = block.timestamp - block.timestamp % 86400 + 28800;
        require(settlePrices[expiry] == 0, "Oracle: already settled");
        settlePrices[expiry] = uint256(getLatestPrice());

        emit Settled(expiry, settlePrices[expiry]);
    }

However, if settle() is not executed on the day of settlement, the settlePrices[expiry] will remains zero, causing all expiring product to be un-burnable/un-redeemable for SmartTrend vaults as they require a non-zero settle price. And in case of DNT vaults, the issue will cause an incorrect payout as maker will get 100% of it.

This issue could occur for L2 like Arbitrum, where the sequencer downtime will prevent any transaction to be included in the block. The impact is high though probability is low as it requires sequencer to be down for >= 24 hours.

Recommendation

It is recommended to set a latestExpiryUpdated variable to track the last day it get updated. Upon updating the next expiry prices, verify if any days were skipped. For any missed days (expiry), assign prices based on the average of the latestExpiryUpdated and the current prices.

Sofa Protocol

Fixed in commit #f9109f.

Code4rena Pro League

Verified to have adopted recommendation to update the missed days since latestExpiryUpdated with linearly extrapolated values.

[M-05] Potential Underflow in getMinterPayoff Function Causing Minter Asset Loss and Stuck Funds

Lines of Code

Description

The collateralAtRiskPercentage can exceed 1e18 in certain cases, leading to potential underflow issues in the getMinterPayoff function. This occurs when params.collateralAtRisk is greater than (totalCollateral - tradingFee), which is possible since it can be equal to totalCollateral (or slightly less).

require(params.collateralAtRisk <= totalCollateral, "Vault: invalid collateral");

The current check allows params.collateralAtRisk to be equal to totalCollateral. When calculating collateralAtRiskPercentage, subtracting the tradingFee from the denominator can make collateralAtRiskPercentage exceed 1e18.

 uint256 collateralAtRiskPercentage = params.collateralAtRisk * 1e18 / (totalCollateral - tradingFee);

In this case if the minter wins, they cannot withdraw their assets due to an underflow when they try to burn thier product ID, since amount * 1e18 will always be less than amount * collateralAtRiskPercentage here:

 payoffShare = payoffShareWithFee - fee + (amount * 1e18 - amount * collateralAtRiskPercentage) / 1e18;

This leads to a loss of assets for the minter and stuck funds in the contract.

Note: This issue is present in all vaults mentioned in Lines of Code section.

Recommendation

Implement the check to prevent params.collateralAtRisk from exceeding (totalCollateral - tradingFee).

Sofa Protocol

Fixed in commit #6fd7.

Code4rena Pro League

Verified. The implemented check ensures that collateralAtRiskPercentage > 0 && collateralAtRiskPercentage <= 1e18.

Lines of Code

Description

The SpotOracle uses chainlink price feeds to record spot prices of assets. This is then used to close valut positions of smart vaults later down the line. The price is read off the latestRoundData function.

(
            /* uint80 roundID */,
            int price,
            /*uint startedAt*/,
            /*uint timeStamp*/,
            /*uint80 answeredInRound*/
        ) = PRICEFEED.latestRoundData();
        require(price > 0, "Oracle: invalid price");

This is then stored in the oracle.

settlePrices[expiry] = uint256(getLatestPrice());

The issue is that chainlink recommends certain checks before using their price feed. The most important one is that of staleness. Prices on chainlink contracts can be outdated due to issues with the oracle network, or high network congestion. To prevent using outdated/stale prices, the recommended fix is to have a hardcoded time threshold and to check the time since the timeStamp of the answer against this threshold. If found to be higher, the price can be categorized as stale.

Using stale prices can lead to a number of issues. Minter or maker position can be settled at incorrect prices and can be denied payouts.

Recommendation

Recommend adding a STALENESS_THRESHOLD value in the contract. Generally this can be set to 1.1-2x the heartbeat value of the pricefeed. Then, record the timestamp of the data and check the time against this threshold.

require((block.timestamp - timeStamp) < STALENESS_THRESHOLD);

Sofa Protocol

Acknowledged.
Our rule is to fetch the Chainlink price every day at 8 AM UTC for settlement. It doesn’t matter much if the Chainlink price is not updated exactly on time. We cannot fetch the price again a few minutes later just because the 8 AM price was not updated within the STALENESS_THRESHOLD. Doing so might result in fetching a price updated after 8 AM, which would still be problematic.

Code4rena Pro League

Customer wishes to maintain a consistent settlement process by settling with Chainlink prices at 8 AM UTC daily regardless of the staleness.

Low Risk Findings (2)

[L-01] Lack of Enforcement on spreadAPR Exceeding borrowAPR in LeverageSmartTrendVault Contract

Lines of Code

Description

In the LeverageSmartTrendVault contract, if spreadAPR exceeds borrowAPR, the mint function will always revert due to an underflow here:

   uint256 borrowFee = minterCollateral * LEVERAGE_RATIO * borrowAPR * (params.expiry - block.timestamp) / SECONDS_IN_YEAR / 1e18;
   uint256 spreadFee = minterCollateral * LEVERAGE_RATIO * spreadAPR * (params.expiry - block.timestamp) / SECONDS_IN_YEAR / 1e18;
>> require(borrowFee - spreadFee >= params.collateralAtRisk - params.makerCollateral, "Vault: invalid collateral at risk");

This will cause a Dos for the contract, making minting a new product impossible.

Recommendation

Add a check to ensure spreadAPR is always less than or equal to borrowAPR in both the initialize function and the updateSpreadAPR function:

+  require(spreadAPR_ <= borrowAPR, "Vault: spreadAPR must be less or equal then borrowAPR");

Sofa Protocol

Acknowledged. We will make sure that borrowAPR is always greater than spreadAPR when they are initialized.

Code4rena Pro League

Customer will enforce the check manually during deployment.

[L-02] Incorrect Assumptions Could Lead to Stuck ETH in the Contract

Lines of Code

Description

The comment above the receive function incorrectly assumes that depositing ETH will help with withdrawals if the balance is insufficient. In reality, ETH should be wrapped to WETH since the vault settles with WETH. Additionally, there is no direct way to withdraw ETH from the contract, making deposited ETH inaccessible unless upgraded.

The withdrawal process relies on WETH availability. If there isn’t enough WETH, the transaction will revert, even if ETH is deposited to cover the needed amount, because the contract tries to withdraw the amount of WETH without checking its availability.

if (payoff > 0) {
    require(POOL.withdraw(address(COLLATERAL), payoff, address(this)) > 0, "Vault: withdraw failed");
>>  WETH.withdraw(payoff);
    payable(_msgSender()).transfer(payoff);
}

Note: This issue is present in all DNT Vaults, making it a critical vulnerability that needs to be addressed promptly.

Recommendation

I recommend to Implement checks to ensure WETH availability before attempting withdrawals. If there is not enough WETH for withdrawals, use the deposited ETH for that purpose if any is available.

if (payoff > 0) {
    require(POOL.withdraw(address(COLLATERAL), payoff, address(this)) > 0, "Vault: withdraw failed");
+     uint wethBalance = WETH.balanceOf(address(this)) ;
+     uint toWithdraw = wethBalance > payoff ? payoff : wethBalance
-    WETH.withdraw(payoff);
+   WETH.withdraw( toWithdraw);
-    payable(_msgSender()).transfer(payoff);
+   require(address(this).balance >= payoff);
+  (bool success, ) = _msgSender().call{value: payoff, gas: 100_000}("");
+   require(success, "Failed to send ETH");
}

Sofa Protocol

Acknowledged. We should make sure users can withdraw the correct payoff. We can transfer some WETH into the vault when it is not enough to withdraw.

Code4rena Pro League

Customer will manually transfer WETH into the vault if required.

Informational Findings (1)

[I-01] Consider adding merkleRoot != 0 check

Lines of Code

Description

The claim() fails to verify that merkleRoot != 0, which could result in an inaccurate revert error.

    function claim(uint256 index, uint256 amount, bytes32[] calldata merkleProof) external {
        require(!isClaimed(index), "MerkleAirdrop: Drop already claimed.");

        bytes32 node = keccak256(abi.encodePacked(_msgSender(), amount));
        require(MerkleProof.verify(merkleProof, merkleRoots[index], node), "MerkleAirdrop: Invalid proof.");

        _setClaimed(index);

        token.mint(_msgSender(), amount);

        emit Claimed(index, _msgSender(), amount);
    }

Recommendation

Recommend to add a check for merkleRoot != 0, same as what is done for claimMultiple().

Sofa Protocol

Fixed in commit #2260.

Code4rena Pro League

Verified that the recommendation has been adopted to check for 'merkleRoot != 0.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.