Skip to content

Latest commit

 

History

History
276 lines (198 loc) · 19.4 KB

56.md

File metadata and controls

276 lines (198 loc) · 19.4 KB
Raw
title sponsor slug date findings contest
yAxis contest
yAxis
2021-11-yaxis
2022-01-27
56

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 code contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the code contest outlined in this document, C4 conducted an analysis of yAxis contest smart contract system written in Solidity. The code contest took place between November 16—November 18 2021.

Wardens

18 Wardens contributed reports to the yAxis contest:

  1. WatchPug (jtp and ming)
  2. harleythedog
  3. cmichel
  4. pauliax
  5. TimmyToes
  6. defsec
  7. ye0lde
  8. hickuphh3
  9. jonah1005
  10. pmerkleplant
  11. 0x0x0x
  12. gzeon
  13. tqts
  14. hubble (ksk2345 and shri4net)
  15. pants
  16. xxxxx
  17. hack3r-0m
  18. Meta0xNull

This contest was judged by 0xleastwood.

Final report assembled by itsmetechjay and CloudEllie.

Summary

The C4 analysis yielded an aggregated total of 15 unique vulnerabilities and 67 total findings. All of the issues presented here are linked back to their original finding.

Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity, 1 received a risk rating in the category of MEDIUM severity, and 12 received a risk rating in the category of LOW severity.

C4 analysis also identified 19 non-critical recommendations and 33 gas optimizations.

Scope

The code under review can be found within the C4 yAxis contest repository, and is composed of 184 smart contracts written in the Solidity programming language and includes 9265 lines of Solidity code and 7712 lines of JavaScript.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low.

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

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

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (2)

[H-01] YaxisVaultAdapter.sol#withdraw() will most certainly fail

Submitted by WatchPug

The actual token withdrawn from vault.withdraw() will most certainly less than the _amount, due to precision loss in _tokensToShares() and vault.withdraw().

As a result, IDetailedERC20(_token).safeTransfer(_recipient, _amount) will revert due to insufficant balance.

Based on the simulation we ran, it will fail 99.99% of the time unless the pps == 1e18.

https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/alchemix/adapters/YaxisVaultAdapter.sol#L68-L72

function withdraw(address _recipient, uint256 _amount) external override onlyAdmin {
    vault.withdraw(_tokensToShares(_amount));
    address _token = vault.getToken();
    IDetailedERC20(_token).safeTransfer(_recipient, _amount);
}

https://github.com/code-423n4/2021-11-yaxis/blob/146febcb61ae7fe20b0920849c4f4bbe111c6ba7/contracts/v3/Vault.sol#L181-L187

function withdraw(
    uint256 _shares
)
    public
    override
{
    uint256 _amount = (balance().mul(_shares)).div(IERC20(address(vaultToken)).totalSupply());

Recommendation

Change to:

function withdraw(address _recipient, uint256 _amount) external override onlyAdmin {
    address _token = vault.getToken();
    uint256 beforeBalance = IDetailedERC20(_token).balanceOf(address(this));
    
    vault.withdraw(_tokensToShares(_amount));

    IDetailedERC20(_token).safeTransfer(
        _recipient,
        IDetailedERC20(_token).balanceOf(address(this)) - beforeBalance
    );
}

Xuefeng-Zhu (yAxis) confirmed

[H-02] CDP.sol update overwrites user's credit on every positive increment

Submitted by harleythedog

Impact

Within CDP.sol (https://github.com/code-423n4/2021-11-yaxis/blob/main/contracts/v3/alchemix/libraries/alchemist/CDP.sol) there is a function called update. This function slowly decreases the debt of a position as yield is earned, until the debt is fully paid off, and the idea is then that the credit should begin incrementing as more yield is accumulated. However, the current logic to increment the totalCredit is this line of code (line 39 of CDP.sol):

\_self.totalCredit = \_earnedYield.sub(\_currentTotalDebt);

Notice that that each time update is called, this overwrites the previous totalCredit with the incremental credit accumulated. The line should instead be:

\_self.totalCredit = \_self.totalCredit.add(\_earnedYield.sub(\_currentTotalDebt));

Indeed, look at the function getUpdatedTotalCredit, it returns the value:

\_self.totalCredit + (\_unclaimedYield - \_currentTotalDebt);

So it is obviously intended that the totalCredit should keep increasing over time instead of being overwritten on each update with a small value. The impact of this issue is large - the credit of every position will always be overwritten and the correct information will be lost forever. User's credit should grow over time, but instead it is overwritten with a small value every time update is called.

Proof of Concept

See line 39 in CDP.sol here: https://github.com/code-423n4/2021-11-yaxis/blob/main/contracts/v3/alchemix/libraries/alchemist/CDP.sol#:~:text=_self.totalCredit%20%3D%20_earnedYield.sub(_currentTotalDebt)%3B

Tools Used

Manual inspection.

Recommended Mitigation Steps

Change code as described above to increment totalCredit instead of overwrite it.

Xuefeng-Zhu (yAxis) disputed:

If there is debt, the credit should be zero

0xleastwood (judge) commented:

It seems like if _self.totalDebt is already zero and yield has been earned by the protocol, _self.totalCredit will be overwritten. This doesn't seem ideal, could you clarify why the issue is incorrect?

0xleastwood (judge) commented:

If I'm not mistaken, yield can be earned from a positive credit (net 0 debt) position.

Xuefeng-Zhu (yAxis) commented:

@0xleastwood totalCredit is 0 if there is debt

0xleastwood (judge) commented:

After chatting to @Xuefeng-Zhu in Discord, he was able to confirm the issue as valid. So keeping it as is.

Medium Risk Findings (1)

[M-01] Prevent Minting During Emergency Exit

Submitted by TimmyToes, also found by defsec

Impact

Potential increased financial loss during security incident.

Proof of Concept

https://github.com/code-423n4/2021-11-yaxis/blob/0311dd421fb78f4f174aca034e8239d1e80075fe/contracts/v3/alchemix/Alchemist.sol#L611

Consider a critical incident where a vault is being drained or in danger of being drained due to a vulnerability within the vault or its strategies.

At this stage, you want to trigger emergency exit and users want to withdraw their funds and repay/liquidate to enable the withdrawal of funds. However, minting against debt does not seem like a desirable behaviour at this time. It only seems to enable unaware users to get themselves into trouble by locking up their funds, or allow an attacker to do more damage.

Recommended Mitigation Steps

Convert emergency exit check to a modifier, award wardens who made that suggestion, and then apply that modifier here.

Alternatively, it is possible that the team might want to allow minting against credit: users minting against credit would effectively be cashing out their rewards. This might be seen as desirable during emergency exit, or it might be seen as a potential extra source of risk. If this is desired, then the emergency exit check could be placed at line 624 with a modified message, instructing users to only use credit.

Xuefeng-Zhu (yAxis) confirmed

Low Risk Findings (12)

Non-Critical Findings (19)

Gas Optimizations (33)

Disclosures

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

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest 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.