Amun contest - defsec's results

We build tokens to make it easy to invest in crypto.

General Information

Platform: Code4rena

Start Date: 13/12/2021

Pot Size: $75,000 USDC

Total HM: 11

Participants: 30

Period: 7 days

Judge: leastwood

Total Solo HM: 4

Id: 68

League: ETH

Amun

Findings Distribution

Researcher Performance

Rank: 12/30

Findings: 3

Award: $1,484.76

🌟 Selected for report: 8

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Amun

Findings Information

🌟 Selected for report: cmichel

Also found by: JMukesh, WatchPug, defsec, p4st13r4

Labels

bug

duplicate

2 (Med Risk)

Awards

394.2378 USDC - $394.24

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

Proof of Concept

Navigate to the following contracts.
transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L104

Tools Used

Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

#0 - loki-sama
2022-01-04T10:34:33Z

duplicate #232

#1 - 0xleastwood
2022-01-22T04:09:12Z

Duplicate of #192

Findings Information

🌟 Selected for report: sirhashalot

Also found by: GiveMeTestEther, JMukesh, Ruhum, WatchPug, defsec, robee

Labels

bug

duplicate

1 (Low Risk)

Awards

76.0316 USDC - $76.03

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

Navigate to the following contracts.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L53

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L43

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L44

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L55

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManager.sol#L131

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV2.sol#L70

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L75

Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.1/contracts/token/ERC20/utils/SafeERC20.sol#L74

#0 - loki-sama
2022-01-04T11:17:32Z

duplicate #294

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: certora, defsec, hyh, kenzo, pedroais

Labels

bug

duplicate

1 (Low Risk)

Awards

98.5594 USDC - $98.56

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Native fund transfers into the basket contracts are only expected from the wrapped token contract. Hence, it would be good to restrict incoming fund transfers to prevent accidental native fund transfers from other sources.

Proof of Concept

Navigate to the following contract code.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L19

On the all basket contracts, The following comment has been written.

    // WETH or WAVAX ...
    IERC20 public immutable INTERMEDIATE_TOKEN;

Tools Used

None

Recommended Mitigation Steps

receive() external payable {
require(msg.sender == address(WETH) | msg.sender == address(WAVAX), 'only wrapped eth');
}

#0 - 0xleastwood
2022-01-23T05:25:13Z

Duplicate of #253

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Auditors

Browse

Contests

Browse

Get in touch

Contact Twitter

Findings Information

🌟 Selected for report: sirhashalot

Also found by: GiveMeTestEther, JMukesh, Ruhum, WatchPug, defsec, robee

Labels

bug

duplicate

1 (Low Risk)

Awards

76.0316 USDC - $76.03

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

The following contract functions performs an ERC20.approve() call but does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

Proof of Concept

Navigate to the following contracts.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L53

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L43

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L44

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L55

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManager.sol#L131

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV2.sol#L70

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L75

Tokens that don't actually perform the approve and return false are still counted as a correct approve.

Tools Used

None

Recommended Mitigation Steps

Its recommend to using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handles the return value check as well as non-standard-compliant tokens.

Reference : https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.1/contracts/token/ERC20/utils/SafeERC20.sol#L74

#0 - loki-sama
2022-01-04T11:17:32Z

duplicate #294

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: certora, defsec, hyh, kenzo, pedroais

Labels

bug

duplicate

1 (Low Risk)

Awards

98.5594 USDC - $98.56

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Proof of Concept

Navigate to the following contract code.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L19

On the all basket contracts, The following comment has been written.

    // WETH or WAVAX ...
    IERC20 public immutable INTERMEDIATE_TOKEN;

Tools Used

None

Recommended Mitigation Steps

receive() external payable {
require(msg.sender == address(WETH) | msg.sender == address(WAVAX), 'only wrapped eth');
}

#0 - 0xleastwood
2022-01-23T05:25:13Z

Duplicate of #253

Findings Information

🌟 Selected for report: WatchPug

Also found by: defsec, gzeon, hyh

Labels

bug

duplicate

1 (Low Risk)

Awards

182.5175 USDC - $182.52

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Amun protocol allows different tokens to be used as output token. The Basket contracts do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L133

Tools Used

Code Review

Recommended Mitigation Steps

Make sure output token for any rebasing/inflation/deflation Add support in contracts for such tokens before accepting user-supplied tokens.

uint256 balanceBefore = getOwnBalance(outputToken);
require(outputToken.safeTransfer(assetId, router, address(this), amount, "Transfer ERC20_TRANSFER_FAILED");
uint256 receivedAmount = getOwnBalance(outputToken) - balanceBefore;

#0 - 0xleastwood
2022-01-23T05:26:28Z

Duplicate of #220

Findings Information

🌟 Selected for report: cmichel

Also found by: Ruhum, defsec

Labels

bug

duplicate

1 (Low Risk)

Awards

270.3963 USDC - $270.40

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

joinTokenEth function performs a low-level status code and parses the return data even if the call failed as it does not check the first return value (success). It could be the case that non-zero data is returned even though the call failed, and the function would return true.

Proof of Concept

Navigate to the following contracts.

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


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

Tools Used

None

Recommended Mitigation Steps

Check the return value or perform a high-level call using the call.value function.


(bool success, ) = _target.call{value: _value}(_calldata);
        require(success, "CALL_FAILED");

#0 - 0xleastwood
2022-01-23T05:20:08Z

Duplicate of #190

Findings Information

🌟 Selected for report: 0x0x0x

Also found by: Dravee, WatchPug, defsec, gzeon, kenzo, pmerkleplant, robee

Labels

bug

duplicate

G (Gas Optimization)

Awards

4.6832 USDC - $4.68

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

Navigate to the following smart contract line.

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/callManagers/RebalanceManager.sol#L218

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/callManagers/RebalanceManager.sol#L234

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/callManagers/RebalanceManagerV2.sol#L155

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L166

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/factories/PieFactoryContract.sol#L88

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Call/CallFacet.sol#L55

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L50

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L160

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L321

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L348

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L381

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L69

Tools Used

None

Recommended Mitigation Steps

Consider to cache array length.

#0 - 0xleastwood
2022-01-23T22:17:47Z

Duplicate of #249

Findings Information

🌟 Selected for report: defsec

Also found by: Jujic, WatchPug

Labels

bug

G (Gas Optimization)

Awards

21.1495 USDC - $21.15

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Proof of Concept

The contest repository contracts contain different pragma versions. The contracts pragma version can be updated to 0.8.4 for the gas optimization.

Affected Contracts

"All Contracts" - Pragma version 0.7.5 is used in the all contracts. (https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoinV2.sol)

Tools Used

None

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.4.

Findings Information

🌟 Selected for report: defsec

Also found by: robee

Labels

bug

G (Gas Optimization)

Awards

35.2492 USDC - $35.25

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

Revert strings > 32 bytes are here:

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/factories/PieFactoryContract.sol#L79

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/facets/shared/Reentry/ReentryProtection.sol#L16

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/bridge/contracts/amunBasketBridge/PolygonERC20Wrapper.sol#L23

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/bridge/contracts/amunBasketBridge/PolygonERC20Wrapper.sol#L28

Tools Used

Manual Review

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.

Findings Information

🌟 Selected for report: defsec

Labels

bug

G (Gas Optimization)

Awards

78.3316 USDC - $78.33

External Links

Link to GitHub issue

Handle

defsec

Vulnerability details

Impact

Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint16 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof of Concept

Navigate to the following contracts.

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

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L37

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L36

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L27

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L143

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L188

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/facets/ERC20/ERC20Facet.sol#L63

Tools Used

None

Recommended Mitigation Steps

Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with 16.

Amun contest - defsec's results

We build tokens to make it easy to invest in crypto.

General Information

Findings Distribution

Researcher Performance

External Links

[M-07] Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom - $394.24

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - loki-sama2022-01-04T10:34:33Z

#1 - 0xleastwood2022-01-22T04:09:12Z

QA Report - $627.51

[L-04] ERC20 approve method missing return value check - $76.03

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - loki-sama2022-01-04T11:17:32Z

[L-10] Restrict funds receivable to be only from wrapped native token - $98.56

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - 0xleastwood2022-01-23T05:25:13Z

Gas Report - $463.01

QA Report - $627.51

[L-04] ERC20 approve method missing return value check - $76.03

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - loki-sama2022-01-04T11:17:32Z

[L-10] Restrict funds receivable to be only from wrapped native token - $98.56

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - 0xleastwood2022-01-23T05:25:13Z

Gas Report - $463.01

[L-12] Incompatibility With Rebasing/Deflationary/Inflationary tokens - $182.52

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

#0 - loki-sama
2022-01-04T10:34:33Z

#1 - 0xleastwood
2022-01-22T04:09:12Z

#0 - loki-sama
2022-01-04T11:17:32Z

#0 - 0xleastwood
2022-01-23T05:25:13Z

#0 - loki-sama
2022-01-04T11:17:32Z

#0 - 0xleastwood
2022-01-23T05:25:13Z

#0 - 0xleastwood
2022-01-23T05:26:28Z

#0 - 0xleastwood
2022-01-23T05:20:08Z

#0 - 0xleastwood
2022-01-23T22:17:47Z