Skip to content

Latest commit

 

History

History
966 lines (594 loc) · 46.1 KB

68.md

File metadata and controls

966 lines (594 loc) · 46.1 KB
title sponsor slug date findings contest
Amun contest
Amun
2021-12-amun
2023-03-07
68

Overview

Please note: Code4rena is an organization that puts learning at the forefront of everything we do. Our rules and processes continue to develop over time, and older reports may reflect previous iterations of these rules and processes. For a more current representation of Code4rena’s severity standardization rules and comprehensive judging criteria, we recommend browsing the reports from C4’s most recent contests.

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 Amun smart contract system written in Solidity. The code contest took place between December 13—December 19 2021.

Wardens

30 Wardens contributed reports to the Amun contest:

  1. WatchPug (jtp and ming)
  2. Czar102
  3. certora
  4. csanuragjain
  5. cmichel
  6. kenzo
  7. robee
  8. harleythedog
  9. JMukesh
  10. gpersoon
  11. hyh
  12. defsec
  13. pauliax
  14. pedroais
  15. jayjonah8
  16. pmerkleplant
  17. p4st13r4 (0xb4bb4 and 0x69e8)
  18. gzeon
  19. GiveMeTestEther
  20. Ruhum
  21. Jujic
  22. itsmeSTYJ
  23. ye0lde
  24. 0x1f8b
  25. sirhashalot
  26. Dravee
  27. 0x0x0x
  28. saian
  29. shenwilly
  30. hubble (ksk2345 and shri4net)

This contest was judged by 0xleastwood.

Final report assembled by burgertime and CloudEllie.

Summary

The C4 analysis yielded an aggregated total of 35 unique vulnerabilities and 129 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, 10 received a risk rating in the category of MEDIUM severity, and 23 received a risk rating in the category of LOW severity.

C4 analysis also identified 44 non-critical recommendations and 50 gas optimizations.

Scope

The code under review can be found within the C4 Amun contest repository, and is composed of 45 smart contracts written in the Solidity programming language and includes 585 lines of Solidity code.

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)

Submitted by WatchPug

Under certain circumstances, e.g. annualizedFee being minted to feeBeneficiary between the time user sent the transaction and the transaction being packed into the block and causing amounts of underlying tokens for each basketToken to decrease. It’s possible or even most certainly that there will be some leftover basket underlying tokens, as BasketFacet.sol#joinPool() will only transfer required amounts of basket tokens from Join contracts.

However, in the current implementation, only the leftover inputToken is returned.

As a result, the leftover underlying tokens won’t be returned to the user, which constitutes users’ fund loss.

SingleTokenJoinV2.sol L57-L78

function joinTokenSingle(JoinTokenStructV2 calldata \_joinTokenStruct)
 external
{
 // ######## INIT TOKEN #########
 IERC20 inputToken = IERC20(\_joinTokenStruct.inputToken);

 inputToken.safeTransferFrom(
 msg.sender,
 address(this),
 \_joinTokenStruct.inputAmount
 );

 \_joinTokenSingle(\_joinTokenStruct);

 // ######## SEND TOKEN #########
 uint256 remainingIntermediateBalance = inputToken.balanceOf(
 address(this)
 );
 if (remainingIntermediateBalance > 0) {
 inputToken.safeTransfer(msg.sender, remainingIntermediateBalance);
 }
}

BasketFacet.sol L143-L168

function joinPool(uint256 \_amount, uint16 \_referral)
 external
 override
 noReentry
{
 require(!this.getLock(), "POOL\_LOCKED");
 chargeOutstandingAnnualizedFee();
 LibBasketStorage.BasketStorage storage bs =
 LibBasketStorage.basketStorage();
 uint256 totalSupply = LibERC20Storage.erc20Storage().totalSupply;
 require(
 totalSupply.add(\_amount) <= this.getCap(),
 "MAX\_POOL\_CAP\_REACHED"
 );

 uint256 feeAmount = \_amount.mul(bs.entryFee).div(10\*\*18);

 for (uint256 i; i < bs.tokens.length; i++) {
 IERC20 token = bs.tokens[i];
 uint256 tokenAmount =
 balance(address(token)).mul(\_amount.add(feeAmount)).div(
 totalSupply
 );
 require(tokenAmount != 0, "AMOUNT\_TOO\_SMALL");
 token.safeTransferFrom(msg.sender, address(this), tokenAmount);
 }
 ...

Furthermore, the leftover tokens in the SingleTokenJoinV2 contract can be stolen by calling joinTokenSingle() with fake outputBasket contract and swap.exchange contract.

Recommended Mitigation Steps

Consider:

  1. Calling IBasketFacet.calcTokensForAmount() first and only swap for exactly the desired amounts of tokens (like SingleTokenJoin.sol);
  2. Or, refund leftover tokens.

loki-sama (Amun) acknowledged

Submitted by Czar102, also found by csanuragjain

Impact

When enough basket token owners exit, it will be impossible to exit pool with the last MIN_AMOUNT tokens because of this check. This will result in locking some tokens forever.

Recommended Mitigation Steps

Consider resigning from this check or performing it only for the owner balance, who would need to have at least MIN_AMOUNT tokens.

loki-sama (Amun) disagreed with severity

0xleastwood (Judge) commented:

Nice find! I think this is valid:)

Medium Risk Findings (10)

Submitted by pmerkleplant, also found by certora, hyh, p4st13r4, pauliax, robee, and WatchPug

Impact

There’s a griefing attack vulnerability in the function joinTokenSingle in SingleTokenJoin.sol as well as SingleTokenJoinV2.sol which makes any user transaction fail with “FAILEDOUTPUTAMOUNT”.

Proof of Concept

The JoinTokenStruct argument for joinTokenSingle includes a field outputAmount to indicate the amount of tokens the user should receive after joining a basket (see line 135 and 130).

However, this amount is compared to the contract’s balance of the token and reverts if the amount is unequal.

If an attacker sends some amount of a basket’s token to the contract, every call to this function will fail as long as the output token equals the attacker’s token send.

Recommended Mitigation Steps

Refactor the require statement to expect at least the outputAmount of tokens, i.e. require(outputAmount >= _joinTokenStruct.outputAmount).

loki-sama (Amun) confirmed

Submitted by JMukesh, also found by certora

Impact

The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoinV2.sol#L26

Recommended Mitigation Steps

add condition to check return value

loki-sama (Amun) confirmed and disagreed with severity

0xleastwood (Judge) commented:

Nice find! I think this could be marked as medium as it leaks value from the protocol but it doesn’t result in assets being lost directly. It requires _INTERMEDIATE_TOKEN to point to a contract which fails upon wrapping the ETH amount.

So considering that _INTERMEDIATE_TOKEN must be improperly set, I will mark this as medium.

Submitted by Czar102

Impact

The initialization status is defined by the name and symbol. It is possible it set them back to an empty string, uninitializing the contract and letting the initialize(..) function be called again. This way, the owner may, for example, hide minting additional tokens. Or, after accidentally setting name and symbol to empty strings, anyone can take control over the contract and mint any number of tokens.

In general, it shouldn’t be possible to initialize more than once.

Recommended Mitigation Steps

Consider adding empty string checks in setName(...) and setSymbol(...) functions.

loki-sama (Amun) confirmed

Submitted by Czar102

Impact

The APY of the annualized fee is dependent on the frequency of the execution of the BasketFacet::chargeOutstandingAnnualizedFee(). If it is called more frequently, the compounding is more frequent and the APY is higher. For less used baskets, the APY might be lower, because the compounding will happen at lower rate.

Recommended Mitigation Steps

Consider calculating the fee as the compounding was continous or with a constant compounding period.

loki-sama (Amun) acknowledged

0xleastwood (Judge) commented:

Nice find!

Submitted by Czar102, also found by gpersoon, gzeon, kenzo, and WatchPug

Impact

Total supply of the token may exceed the maxCap introduced. This can happen when a user wants to join the pool. The check in BasketFacet::joinPool(...) includes only the base amount, without fee. Thus, if fee is on and someone will want to create as many tokens as possible, the totalSupply + _amount will be set to maxCap. The call will succeed, but new tokens were also minted as the fee for bs.feeBeneficiary if bs.entryFee and bs.entryFeeBeneficiaryShare are nonzero. Thus, the number of tokens may exceed maxCap.

Recommended Mitigation Steps

Consider calculating feeAmount and feeBeneficiaryShare before the require(...) statement and check totalSupply.add(_amount).add(feeBanficiaryShare) <= this.getCap().

loki-sama (Amun) acknowledged

Submitted by gpersoon, also found by kenzo and robee

Impact

Some functions, like rebalance() in RebalanceManagerV3 use _deadline as a time limit for swapExactTokensForTokens() Other functions, like _joinTokenSingle() of SingleTokenJoinV2.sol and _exit() of SingleNativeTokenExitV2() use block.timestamp, although a deadline field is present in the struct.

Possibly the deadline fields should have been used.

Proof of Concept

RebalanceManagerV3.sol L158-L203

function rebalance(UnderlyingTrade[] calldata \_swapsV2, uint256 \_deadline) external override onlyRebalanceManager {
...
 for (uint256 i; i < \_swapsV2.length; i++) {
 ...
 for (uint256 j; j < trade.swaps.length; j++) {
 ..
 \_swapUniswapV2(swap.exchange,input,0, swap.path,address(basket), \_deadline );

RebalanceManagerV3.sol L63-L104

function \_swapUniswapV2(...) {
 basket.singleCall(
 exchange,
 abi.encodeWithSelector( IUniswapV2Router02(exchange).swapExactTokensForTokens.selector, quantity, minReturn, path, recipient, deadline ),
 0
 );

SingleTokenJoinV2.sol L80-L112

struct JoinTokenStructV2 {
 ...
 uint256 deadline;
 ...
 }
function \_joinTokenSingle(JoinTokenStructV2 calldata \_joinTokenStruct) internal {
 ...
 for (uint256 j; j < trade.swaps.length; j++) {
 IPangolinRouter(swap.exchange).swapExactTokensForTokens( amountIn, 0, swap.path, address(this), block.timestamp );
 }
 }

SingleNativeTokenExitV2.sol L59-L88

 struct ExitTokenStructV2 {
 ...
 uint256 deadline;
 ...
 }
function \_exit(ExitTokenStructV2 calldata \_exitTokenStruct) internal {
 ...
 for (uint256 i; i < \_exitTokenStruct.trades.length; i++) {
 ...
 for (uint256 j; j < trade.swaps.length; j++) {
 ...
 IPangolinRouter(swap.exchange).swapExactTokensForTokens( IERC20(swap.path[0]).balanceOf(address(this)), 0, swap.path, address(this), block.timestamp );
 }
 }

Recommended Mitigation Steps

Check whether the deadline fields should have been used. If so replace block.timestamp with the appropriate deadline

loki-sama (Amun) confirmed

Submitted by cmichel, also found by defsec, JMukesh, p4st13r4, and WatchPug

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

See:

  • SingleNativeTokenExitV2.exit’s outputToken.transfer(msg.sender, outputTokenBalance);
  • PieFactoryContract.bakePie’s pie.transfer(msg.sender, _initialSupply);

Impact

Tokens that don’t actually perform the transfer and return false are still counted as a correct transfer and the tokens remain in the SingleNativeTokenExitV2 contract and could potentially be stolen by someone else.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

0xleastwood (Judge) commented:

Nice find! I think this is valid considering the extent basket tokens are used. It is more than likely that non-standard tokens will be utilised.

Submitted by kenzo, also found by cmichel and hyh

SingleNativeTokenExitV2 allows the user to exit and execute trades via multiple exchanges. When finishing the trades and sending a single output token back to the user, the contract takes that token from the last swap in the first exchange’s trades. There is nothing in the struct that signifies this will be the output token, and this also impairs the exit functionality.

Impact

Let’s say a basket only holds token TOKE, and user would like to exit to DAI. But there’s no exchange with good liquidity for TOKE -> DAI. So the user crafts a trade to exchange TOKE for WOKE in exchange A, and then exchange WOKE for DAI in exchange B, to finally receive back DAI. The contract will not let him do so, as the output token is taken to be the output token of the first exchange - WOKE in our example.

Proof of Concept

In exit, the output token is taken to be the last token exchanged in the first exchange: (Code ref)

address[] calldata path = \_exitTokenStruct
 .trades[0]
 .swaps[\_exitTokenStruct.trades[0].swaps.length - 1]
 .path;
 IERC20 outputToken = IERC20(path[path.length - 1]); //this could be not the target token

This manifests the issue I detailed above.

Recommended Mitigation Steps

Have the outputToken be a parameter supplied in ExitTokenStructV2.

loki-sama (Amun) acknowledged

Submitted by harleythedog

Impact

The CallFacet.sol contract has the function _call :

function \_call(
 address \_target,
 bytes memory \_calldata,
 uint256 \_value
) internal {
 require(address(this).balance >= \_value, "ETH\_BALANCE\_TOO\_LOW");
 (bool success, ) = \_target.call{value: \_value}(\_calldata);
 require(success, "CALL\_FAILED");
 emit Call(msg.sender, \_target, \_calldata, \_value);
}

This function is utilized in a lot of different places. According to the Solidity docs, “The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed”.

As a result, it is possible that this call will not work but _call will not notice anything went wrong. It could be possible that a user is interacting with an exchange or token that has been deleted, but _call will not notice that something has gone wrong and as a result, ether can become stuck in the contract. For this reason, it would be better to also check for the contract’s existence prior to executing _target.call.

For reference, see a similar high severity reported in a Uniswap audit here (report # 9): https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf

Proof of Concept

See _call here: CallFacet.sol L108.

Recommended Mitigation Steps

To ensure tokens don’t get stuck in edge case where user is interacting with a deleted contract, make sure to check that contract actually exists before calling it.

loki-sama (Amun) confirmed

Submitted by certora

after that fee is calculated, it is minted to the feeBeneficiary. simply minting the exact amount results lower fee than it should be.

Impact

feeBeneficiary will get less fees than it should.

Proof of Concept

let’s assume that the basket assets are worth 1M dollars, and totalSupply = 1M. the result of calcOutStandingAnnualizedFee is 100,00 so the feeBeneficiary should get 100,00 dollars. however, when minting 100,00 the totalSupply will increase to 1,100,000 so they will own 100000/1100000 * (1M dollars) = 90909.09 dollars instead of 100k

loki-sama (Amun) acknowledged:

This is mitigated by the feeBeneficiary diluting his own shares if he gets fees on his fees.

0xleastwood (Judge) asked:

I’m not exactly sure if I understand what the warden is stating here. Could you confirm @loki-sama ?

loki-sama (Amun) confirmed:

Ok, I myself misunderstood. He is correct that we don’t get the full value. When we take a fee of 10% like from his example. What we do is mint 10% of the basket to ourselves. That 10% after minting is not holding 10% of the underling.

Low Risk Findings (23)

Non-Critical Findings (44)

Gas Optimizations (50)

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.

.grvsc-container { overflow: auto; position: relative; -webkit-overflow-scrolling: touch; padding-top: 1rem; padding-top: var(--grvsc-padding-top, var(--grvsc-padding-v, 1rem)); padding-bottom: 1rem; padding-bottom: var(--grvsc-padding-bottom, var(--grvsc-padding-v, 1rem)); border-radius: 8px; border-radius: var(--grvsc-border-radius, 8px); font-feature-settings: normal; line-height: 1.4; }

.grvsc-code { display: table; }

.grvsc-line { display: table-row; box-sizing: border-box; width: 100%; position: relative; }

.grvsc-line > * { position: relative; }

.grvsc-gutter-pad { display: table-cell; padding-left: 0.75rem; padding-left: calc(var(--grvsc-padding-left, var(--grvsc-padding-h, 1.5rem)) / 2); }

.grvsc-gutter { display: table-cell; -webkit-user-select: none; -moz-user-select: none; user-select: none; }

.grvsc-gutter::before { content: attr(data-content); }

.grvsc-source { display: table-cell; padding-left: 1.5rem; padding-left: var(--grvsc-padding-left, var(--grvsc-padding-h, 1.5rem)); padding-right: 1.5rem; padding-right: var(--grvsc-padding-right, var(--grvsc-padding-h, 1.5rem)); }

.grvsc-source:empty::after { content: ' '; -webkit-user-select: none; -moz-user-select: none; user-select: none; }

.grvsc-gutter + .grvsc-source { padding-left: 0.75rem; padding-left: calc(var(--grvsc-padding-left, var(--grvsc-padding-h, 1.5rem)) / 2); }

/* Line transformer styles */

.grvsc-has-line-highlighting > .grvsc-code > .grvsc-line::before { content: ' '; position: absolute; width: 100%; }

.grvsc-line-diff-add::before { background-color: var(--grvsc-line-diff-add-background-color, rgba(0, 255, 60, 0.2)); }

.grvsc-line-diff-del::before { background-color: var(--grvsc-line-diff-del-background-color, rgba(255, 0, 20, 0.2)); }

.grvsc-line-number { padding: 0 2px; text-align: right; opacity: 0.7; }

.dark-default-dark { background-color: #1E1E1E; color: #D4D4D4; } .dark-default-dark .mtk4 { color: #569CD6; } .dark-default-dark .mtk1 { color: #D4D4D4; } .dark-default-dark .mtk11 { color: #DCDCAA; } .dark-default-dark .mtk12 { color: #9CDCFE; } .dark-default-dark .mtk3 { color: #6A9955; } .dark-default-dark .mtk15 { color: #C586C0; } .dark-default-dark .mtk7 { color: #B5CEA8; } .dark-default-dark .mtk8 { color: #CE9178; } .dark-default-dark .grvsc-line-highlighted::before { background-color: var(--grvsc-line-highlighted-background-color, rgba(255, 255, 255, 0.1)); box-shadow: inset var(--grvsc-line-highlighted-border-width, 4px) 0 0 0 var(--grvsc-line-highlighted-border-color, rgba(255, 255, 255, 0.5)); }