Angle Protocol - Invitational - auditor0517's results

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Redeemer.sol#L131

Vulnerability details

Impact

Redeemers might charge more collaterals during redemption/swap by the reentrancy attack.

Proof of Concept

Redeemers can redeem the agToken for collaterals in Redeemer contract and _redeem() burns the agToken and transfers the collaterals.

    function _redeem(
        uint256 amount,
        address to,
        uint256 deadline,
        uint256[] memory minAmountOuts,
        address[] memory forfeitTokens
    ) internal returns (address[] memory tokens, uint256[] memory amounts) {
        TransmuterStorage storage ts = s.transmuterStorage();
        if (ts.isRedemptionLive == 0) revert Paused();
        if (block.timestamp > deadline) revert TooLate();
        uint256[] memory subCollateralsTracker;
        (tokens, amounts, subCollateralsTracker) = _quoteRedemptionCurve(amount);
        // Updating the normalizer enables to simultaneously and proportionally reduce the amount
        // of stablecoins issued from each collateral without having to loop through each of them
        _updateNormalizer(amount, false);

        IAgToken(ts.agToken).burnSelf(amount, msg.sender); //@audit-info burn agToken

        address[] memory collateralListMem = ts.collateralList;
        uint256 indexCollateral;
        for (uint256 i; i < amounts.length; ++i) {
            if (amounts[i] < minAmountOuts[i]) revert TooSmallAmountOut();
            // If a token is in the `forfeitTokens` list, then it is not sent as part of the redemption process
            if (amounts[i] > 0 && LibHelpers.checkList(tokens[i], forfeitTokens) < 0) {
                Collateral storage collatInfo = ts.collaterals[collateralListMem[indexCollateral]];
                if (collatInfo.onlyWhitelisted > 0 && !LibWhitelist.checkWhitelist(collatInfo.whitelistData, to))
                    revert NotWhitelisted();
                if (collatInfo.isManaged > 0)
                    LibManager.release(tokens[i], to, amounts[i], collatInfo.managerData.config);
                else IERC20(tokens[i]).safeTransfer(to, amounts[i]); //@audit reentrancy
            }
            if (subCollateralsTracker[indexCollateral] - 1 <= i) ++indexCollateral;
        }
        emit Redeemed(amount, tokens, amounts, forfeitTokens, msg.sender, to);
    }

During the collateral transfers(direct transfer or in LibManager.release()), there might be a hook for the recipient in the case of ERC777 tokens.

Then the recipient might charge more collaterals by reentrancy like this.

1. Let's suppose there are 2 collaterals colA and colB. The transmuter contract contains 1000 amounts of colA and colB. Alice has 20 amounts of agToken.
2. At the first time, Alice calls redeem() with 10 amounts of agToken and she should receive 10 amounts of colA and colB.
3. As colA is an ERC777 token, she calls redeem(10) again inside the hook after the colA transfer.
4. During the second redemption, total collaterals will be colA = 990, colB = 1000 because colB isn't transferred in the first redemption yet.
5. After all, Alice will receive more collaterals in the second redemption from this calculation.

I think a similar reentrancy attack might be possible during the swap as well.

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should add the nonReentrant modifier to the major functions like redeem()/swap().

Assessed type

Reentrancy

Lines of code

https://github.com/AngleProtocol/merkl-contracts/blob/1825925daef8b22d9d6c0a2bc7aab3309342e786/contracts/Distributor.sol#L234

Vulnerability details

Impact

The first disputer might lose funds although his dispute is valid.

Proof of Concept

Users can dispute the current tree using disputeTree() and the governor refunds the dispute funds if the dispute is valid in resolveDispute().

    function disputeTree(string memory reason) external {
        if (block.timestamp >= endOfDisputePeriod) revert InvalidDispute();
        IERC20(disputeToken).safeTransferFrom(msg.sender, address(this), disputeAmount);
        disputer = msg.sender;
        emit Disputed(reason);
    }

    /// @notice Resolve the ongoing dispute, if any
    /// @param valid Whether the dispute was valid
    function resolveDispute(bool valid) external onlyGovernorOrGuardian {
        if (disputer == address(0)) revert NoDispute();
        if (valid) {
            IERC20(disputeToken).safeTransfer(disputer, disputeAmount);
            // If a dispute is valid, the contract falls back to the last tree that was updated
            _revokeTree();
        } else {
            IERC20(disputeToken).safeTransfer(msg.sender, disputeAmount);
            endOfDisputePeriod = _endOfDisputePeriod(uint48(block.timestamp));
        }
        disputer = address(0);
        emit DisputeResolved(valid);
    }

But disputeTree() can be called again by another disputer although there is an active disputer and resolveDispute() refunds to the last disputer only.

In the worst case, a valid disputer might lose the dispute funds by malicious frontrunners.

1. A valid disputer creates a dispute using disputeTree().
2. As it's valid, the governor calls resolveDispute(valid = true) to accept the dispute and refund the funds.
3. A malicious user calls disputeTree() by front running.
4. Then during resolveDispute(true), the dispute funds will be sent to the second disputer and the first disputer will lose the funds although he's valid.

Tools Used

Manual Review

Recommended Mitigation Steps

disputeTree() shouldn't allow another dispute when there is an active dispute already.

Assessed type

Invalid Validation

Lines of code

https://github.com/AngleProtocol/merkl-contracts/blob/1825925daef8b22d9d6c0a2bc7aab3309342e786/contracts/Distributor.sol#L199

Vulnerability details

Impact

Users might claim rewards using an unconfirmed merkle root.

Proof of Concept

getMerkleRoot() returns the merkle root to claim the rewards.

    function getMerkleRoot() public view returns (bytes32) {
        if (block.timestamp >= endOfDisputePeriod) return tree.merkleRoot;
        else return lastTree.merkleRoot;
    }

It returns the last tree until endOfDisputePeriod as the current tree might be revoked due to the active dispute.

But even after the endOfDisputePeriod, the tree can be revoked if there is an active disputer and the rewards might be claimed unexpectedly.

1. The merkle tree was updated wrongly using updateTree() and one user disputed that using disputeTree().
2. But the governor couldn't resolve the dispute until endOfDisputePeriod due to some reasons.
3. In this case, getMerkleRoot() will return the wrong merkle tree and claim() wouldn't work as expected.

I think it's not an intended behavior to claim rewards using the merkle tree that might be revoked.

Tools Used

Manual Review

Recommended Mitigation Steps

getMerkleRoot() should return the last tree when there is an active disputer.

    function getMerkleRoot() public view returns (bytes32) {
        if (block.timestamp >= endOfDisputePeriod || disputer != address(0)) return tree.merkleRoot;
        else return lastTree.merkleRoot;
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Redeemer.sol#L156-L157 https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibSetters.sol#L230-L240 https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibHelpers.sol#L77-L80

Vulnerability details

Impact

LibHelpers.piecewiseLinear reverts when the value is less than the first element of the array. This method is used in Redeemer contract and if the collateral ratio is below the first element of xRedemptionCurve, the redepmtion will revert.

Proof of Concept

In Redeemer._quoteRedemptionCurve, a penalty factor is applied when the protocol is under-collateralized using LibHelpers.piecewiseLinear.

Redeemer.sol#L156-L157

        uint64[] memory xRedemptionCurveMem = ts.xRedemptionCurve;
        penaltyFactor = uint64(LibHelpers.piecewiseLinear(collatRatio, xRedemptionCurveMem, yRedemptionCurveMem));

xRedemptionCurveMem is strictly increasing and upper bounded by BASE_9, and there's no more limitations.

LibSetters.sol

230        (action == ActionType.Redeem && (xFee[n - 1] > BASE_9 || yFee[n - 1] < 0 || yFee[n - 1] > int256(BASE_9)))
           
233        for (uint256 i = 0; i < n - 1; ++i) {
234            if (
           
240                (action == ActionType.Redeem && (xFee[i] >= xFee[i + 1] || yFee[i] < 0 || yFee[i] > int256(BASE_9)))

So collatRatio can be less than the first element of xRedemptionCurveMem. In that case, LibHelpers.findLowerBound will return 0 and LibHelpers.piecewiseLinear will revert on the following line.

LibHelpers.sol#L77-L80

    return
        yArray[indexLowerBound] +
        ((yArray[indexLowerBound + 1] - yArray[indexLowerBound]) * int64(x - xArray[indexLowerBound])) /
        int64(xArray[indexLowerBound + 1] - xArray[indexLowerBound]);

Redeemer._redeem calls _quoteRedemptionCurve, so the redemption will be blocked in this case.

Tools Used

Manual Review

Recommended Mitigation Steps

We can add the following line to mitigate this issue.

    if (indexLowerBound == 0 && x < xArray[0]) return yArray[0];

Assessed type

Under/Overflow

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibGetters.sol#L87

Vulnerability details

Impact

LibGetters.getCollateralRatio() might return the incorrect ratio due to the unsafe cast.

Proof of Concept

getCollateralRatio() outputs the collateral ratio using the total collaterals and issued agTokens.

    // The `stablecoinsIssued` value need to be rounded up because it is then used as a divizer when computing
    // the amount of stablecoins issued
    stablecoinsIssued = uint256(ts.normalizedStables).mulDiv(ts.normalizer, BASE_27, Math.Rounding.Up);
    if (stablecoinsIssued > 0)
        collatRatio = uint64(totalCollateralization.mulDiv(BASE_9, stablecoinsIssued, Math.Rounding.Up)); //@audit unsafe cast
    else collatRatio = type(uint64).max;

Typically, the collatRatio should be around BASE_9 but the ratio might be larger than type(uint64).max during the initial stage.

Furthermore, totalCollateralization is calculated using the raw balance of collaterals and it might be manipulated when stablecoinsIssued is not large.

Then collatRatio might be cast to the wrong value.

After all, getCollateralRatio() will return the wrong ratio and it will affect the protocol seriously.

Tools Used

Manual Review

Recommended Mitigation Steps

I think we should use the SafeCast library in getCollateralRatio().

Assessed type

Other

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Swapper.sol#L206 https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Redeemer.sol#L131 https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/savings/SavingsVest.sol#L110

Vulnerability details

Impact

The agToken might be minted wrongly as rewards due to the reentrancy attack.

Proof of Concept

There are redeem/swap logics in the transmuter contract and all functions don't have a nonReentrant modifier.

So the typical reentrancy attack is possible during redeem/swap as I mentioned in my other report.

But besides that, the read-only reentrancy attack is possible from the SavingsVest contract, and the agToken might be minted/burnt incorrectly like this.

1. The collatRatio is BASE_9(100%) now and Alice starts a swap from collateral to agToken in Swapper contract.
2. In _swap(), it mints the agToken after depositing the collaterals.

    if (mint) {
        uint128 changeAmount = (amountOut.mulDiv(BASE_27, ts.normalizer, Math.Rounding.Up)).toUint128();
        // The amount of stablecoins issued from a collateral are not stored as absolute variables, but
        // as variables normalized by a `normalizer`
        collatInfo.normalizedStables += uint216(changeAmount);
        ts.normalizedStables += changeAmount;
        if (permitData.length > 0) {
            PERMIT_2.functionCall(permitData);
        } else if (collatInfo.isManaged > 0)
            IERC20(tokenIn).safeTransferFrom(
                msg.sender,
                LibManager.transferRecipient(collatInfo.managerData.config),
                amountIn
            );
        else IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn); //@audit reentrancy
        if (collatInfo.isManaged > 0) {
            LibManager.invest(amountIn, collatInfo.managerData.config);
        }
        IAgToken(tokenOut).mint(to, amountOut);
    }

After depositing the collaterals, Alice might have a hook in the case of ERC777 tokens before the agToken is minted.

3. Then Alice calls SavingsVest.accrue() inside the hook and getCollateralRatio() will return the incorrect ratio as the agToken isn't minted yet.
4. So collatRatio will be larger than the real value and additional rewards would be minted if collatRatio > BASE_9 + BASE_6.

Then Alice would get more rewards from the SavingsVest.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend adding the nonReentrant modifer to getCollateralRatio() as well as redeem()/swap() functions.

Assessed type

Reentrancy

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/savings/SavingsVest.sol#L183

Vulnerability details

Impact

estimatedAPR() might return the wrong APR and it will make users confused.

Proof of Concept

SavingsVest.estimatedAPR() returns the APR using the current vestingProfit and vestingPeriod.

    function estimatedAPR() external view returns (uint256 apr) {
        uint256 currentlyVestingProfit = vestingProfit;
        uint256 weightedAssets = vestingPeriod * totalAssets();
        if (currentlyVestingProfit != 0 && weightedAssets != 0)
            apr = (currentlyVestingProfit * 3600 * 24 * 365 * BASE_18) / weightedAssets;
    }

First of all, it uses the current vestingRatio = vestingProfit / vestingPeriod for 1 year even if vestingPeriod < 1 year. I think it might be an intended behavior to estimate the APR with the current vesting ratio.

But it's wrong to use the same vesting ratio after the vesting period is finished already.

In accrue(), it updates the vestingProfit and lastUpdate only when it's overcollateralized/undercollateralized more than 0.1%.

So lastUpdate wouldn't be changed for a certain time while collatRatio is in range (99.9%, 100.1%).

1. At the first time, vestingProfit = 100, vestingPeriod = 10 days and estimatedAPR() returns the correct value.
2. After 10 days, all vestings are unlocked and there is no locked profit. But accrue() has never been called due to the stable collateral ratio.
3. In estimatedAPR(), vestingProfit will be 100 and it will return the same APR as 10 days before.
4. But the APR should be 0 as there is no locked profit now.

Tools Used

Manual Review

Recommended Mitigation Steps

estimatedAPR() should return 0 when lockedProfit() == 0.

    function estimatedAPR() external view returns (uint256 apr) {
        if (lockedProfit() == 0) return 0; //check locked profit first

        uint256 currentlyVestingProfit = vestingProfit;
        uint256 weightedAssets = vestingPeriod * totalAssets();
        if (currentlyVestingProfit != 0 && weightedAssets != 0)
            apr = (currentlyVestingProfit * 3600 * 24 * 365 * BASE_18) / weightedAssets;
    }

Assessed type

Other

[L-01] setDisputePeriod() should validate disputePeriod > 0

• https://github.com/AngleProtocol/merkl-contracts/blob/1825925daef8b22d9d6c0a2bc7aab3309342e786/contracts/Distributor.sol#L286

    function setDisputePeriod(uint48 _disputePeriod) external onlyGovernorOrGuardian {
        disputePeriod = uint48(_disputePeriod); //@audit should check > 0
        emit DisputePeriodUpdated(_disputePeriod);
    }

If disputePeriod == 0, _endOfDisputePeriod() will return the same time as treeUpdate when it's divisible by _EPOCH_DURATION. Then the merkle tree would be updated without any dispute period.

    function _endOfDisputePeriod(uint48 treeUpdate) internal view returns (uint48) {
        return ((treeUpdate - 1) / _EPOCH_DURATION + 1 + disputePeriod) * (_EPOCH_DURATION);
    }

[L-02] The merkle tree might be updated too soon by the governor.

• https://github.com/AngleProtocol/merkl-contracts/blob/1825925daef8b22d9d6c0a2bc7aab3309342e786/contracts/Distributor.sol#L220

    function updateTree(MerkleTree calldata _tree) external {
        if (
            disputer != address(0) ||
            // A trusted address cannot update a tree right after a precedent tree update otherwise it can de facto
            // validate a tree which has not passed the dispute period
            ((canUpdateMerkleRoot[msg.sender] != 1 || block.timestamp < endOfDisputePeriod) &&
                !core.isGovernorOrGuardian(msg.sender))
        ) revert NotTrusted();

Users can dispute the merkle tree within endOfDisputePeriod but the tree might be confirmed anytime by the governor and it might break the fairness of the reward system.

[L-03] Centralization risk

• https://github.com/AngleProtocol/merkl-contracts/blob/1825925daef8b22d9d6c0a2bc7aab3309342e786/contracts/Distributor.sol#L279

    function recoverERC20(address tokenAddress, address to, uint256 amountToRecover) external onlyGovernorOrGuardian {
        IERC20(tokenAddress).safeTransfer(to, amountToRecover);
        emit Recovered(tokenAddress, to, amountToRecover);
    }

The rewards might be withdrawn by the governor.

[L-04] Wrong comment

• https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Swapper.sol#L390

//                      = (g(0)-1+sqrt[(1+g(0))**2+2M(f_{i+1}-g(0))/b_{i+1})]) @audit divide by 2

• https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Swapper.sol#L402

//                      = (g(0)+1-sqrt[(1-g(0))**2-2M(f_{i+1}-g(0))/b_{i+1})]) @audit divide by 2

[N-01] Variables need not be initialized to zero

• https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/Redeemer.sol#L199
• https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibHelpers.sol#L23
• https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibSetters.sol#L233

I found H3, M12 and QGA for 40 hours.

This audit for Angle protocol consists of two main parts, transmitter, and merkl. The test and the documentation were good and especially the whitepaper was great. I started with the transmuter because it contains vast mathematical logic for the stablecoin trading actions. The transmuter uses the Diamond Proxy pattern and I was familiar with the pattern from the ZkSynk contest.

The main functionalities of the transmuter are redemption and mint/burn. The Redeemer contract implements redemption and it returns all of the backing collaterals while redeeming. During redemption, Redeemer gets the values of collaterals from the oracle and uses those values so this protocol will be robust during black swan events. Mint/burn are handled in the Swapper contract. The main contribution of Swapper is setting fees so they can stabilize the total collateral ratio by manipulating of fee price of mint and burn. We can also specify exact input and output amounts and huge mathematical perspectives are used in whitepaper and implementation. I found a mistake on the whitepaper, but the implementation was good, fortunately.

I moved to the merkl module and it handles the distribution of rewards. Those distribution logics are interesting and I enjoyed finding several vulnerabilities there. I feel Distribution contracts are more centralized because the governor and guardians can affect the reward distribution a lot. There were some instances of array copying and if we resize instead of copy, some gas will be saved.

Time spent:

40 hours