Amun contest - gzeon's results

Handle

gzeon

Vulnerability details

To keep the most precision, it is better to mul before div https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L260

        return
            totalSupply.mul(annualizedFee).div(10**18).mul(timePassed).div(
                365 days
            );

to

        return
            totalSupply.mul(annualizedFee).mul(timePassed).div(10**18).div(
                365 days
            );

Handle

gzeon

Vulnerability details

Impact

In joinPool the amount of token required from the sender is calculated as follow https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L162

           uint256 tokenAmount =
                balance(address(token)).mul(_amount.add(feeAmount)).div(
                    totalSupply
                );
            require(tokenAmount != 0, "AMOUNT_TOO_SMALL");
            token.safeTransferFrom(msg.sender, address(this), tokenAmount);

If token is a fee on transfer token, the basket would receive less than tokenAmount which is undesirable

Proof of Concept

Consider

1. 50:50 basket of $ETH and $SHIT
2. A deposit 1 $ETH and 1 $SHIT for 100 $BASKET
3. $SHIT take 10% fee on transfer
4. Pool now have 1 $ETH and 0.9 $SHIT
5. B deposit 1 $ETH and 0.9 $SHIT for 100 $BASKET
6. Pool now have 2 $ETH and 1.71 $SHIT

A and B got the same amount of $BASKET despite B pay less $SHIT

Recommended Mitigation Steps

Check before and after balance to make sure tokenAmount is received, but it is a bit tricky as how to transfer the right amount of token to account for the fee. Need to make sure user don't add fee-on-transfer token to the basket.

Handle

gzeon

Vulnerability details

e.g. https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L49

        uint256 length = bs.tokens.length;
        // remove token from array
        for (uint256 i; i < length; i++) {
            if (address(bs.tokens[i]) == _token) {
                bs.tokens[i] = bs.tokens[length - 1];
                bs.tokens.pop();
                emit TokenRemoved(_token);
                break;
            }
        }

Handle

gzeon

Vulnerability details

Constant expressions are left as expressions and will recalculate on read, it is recommended to use immutable instead. Ref: https://github.com/ethereum/solidity/issues/9232#issuecomment-646131646

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/LibBasketStorage.sol#L7 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Call/LibCallStorage.sol#L5 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/ERC20/LibERC20Storage.sol#L5