Angle Protocol - Invitational - Lambda's results

Lines of code

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

Vulnerability details

Impact

There are two potential sources of reentrancy within Redeemer._redeem:

• The call to LibManager.release: As this is an arbitrary strategy that may perform arbitrary calls / callbacks on release (for instance because it calls another protocol which supports callbacks on redemptions), so it can be possible to control the control flow there depending on the strategies used.
• The ERC20 transfer: While there are some tokens with transfer hooks (e.g., ERC777), this is rather unusual in practice and is also something that can be controlled by the protocol.

A reentrancy in this place can be exploited by an attacker because balanceOf(address(this)) will not be updated yet (or at least not for all tokens), whereas the normalizer was already decreases. As LibGetters.getCollateralRatio() uses both of these values, it will report a wrong collateralization ratio when reentering.

Proof of Concept

Let's say that there are two tokens (the first one is managed, the second one is not) with current balances A & B, the current collateralization ratio is 95%, and there are X issued stablecoins. Because the used strategy supports callbacks, the user manages to reenter whenever LibManager.release is called. The attacker now performs a first redemption that burns amount (let's say 10,000) tokens and decreases the normalizer by this amount. Now, during the LibManager.release call, the user reenters by performing a second, huge redemption. This redemption will perform this calculation in getCollateralRatio:

stablecoinsIssued = uint256(ts.normalizedStables).mulDiv(ts.normalizer, BASE_27, Math.Rounding.Up);

Because it uses the already updated normalizer, stablecoinsIssued will be X - 10,000. However, when reentering, the token balances will still be A & B, resulting in a higher collateralization ratio (e.g., 100%). Therefore, the user does not have to pay any penalty for this second redemption, although he should (as the system is under-collateralized).

Recommended Mitigation Steps

Do not allow reentering redeem.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The following code is used to calculate the collateralization ratio (when stablecoinsIssued > 0):

collatRatio = uint64(totalCollateralization.mulDiv(BASE_9, stablecoinsIssued, Math.Rounding.Up));

During normal operation, this should not overflow. However, when stablecoinsIssued is very small, a user can donate a small amount to the contract (in order to increase totalCollateralization, which queries balances) and produce an overflow.

Proof of Concept

Let's take an extreme example and say that stablecoinsIssued is 1 (although the attack also works with larger values). If a user now transfer 1 USD of a stablecoin to the contract, totalCollateralization will be roughly $10^{18}$ . Therefore, the system calculates:

collatRatio = uint64(10**18 * 10**9 / 1) = uint64(10**27);

Because 10**27 > type(uint64).max, this overflows. An attacker can also choose the donated amount to produce a targeted collateralization ratio.

Recommended Mitigation Steps

Check if the produced ratio is larger than type(uint64).max. If so, set the ratio to type(uint64).max.

Assessed type

Under/Overflow

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/8a2c3aaf4bd054581b06d33049370a6f01b56d44/contracts/transmuter/libraries/LibSetters.sol#L123 https://github.com/AngleProtocol/angle-transmuter/blob/8a2c3aaf4bd054581b06d33049370a6f01b56d44/contracts/transmuter/facets/Redeemer.sol#L64

Vulnerability details

Impact

The order of ts.collateralList is not stable: Whenever LibSetters.revokeCollateral is used to revoke a collateral, it may change because of the swap that is performed. However, the function Redeemer.redeem relies on this order, as the user has to provide the minAmountsOut in the order of ts.collateralList. This can lead to situations where the user has crafted the minAmountsOut array when the order was still different, leading to unintended results (and potentially redemptions that the user did not want to accept). It also means that revoking a collateral can be challenging for the team / governance because it should never be done when a user has already prepared a redemption (either via the frontend which he had open or some other way to interact with the contract). But there is of course no way to know this.

Proof of Concept

Let's say the system contains the collateral [tokenA, tokenB, tokenC]. normalizedStables for tokenA is 0. The user therefore does not want to receive tokenA (and will not receive anything for it). However, it is extremely important to him that he receives 100,000 of tokenC. He therefore crafts a minAmountsOut of [0, 10000, 100000]. Just before he submits the call, tokenA is removed from the system, resulting in the collateral array [tokenC, tokenB]. Even if the user only receives 50,000 tokens of tokenC, the call will therefore succeed.

Recommended Mitigation Steps

The problem could be alleviated a bit by checking the length of minAmountsOut (making sure it is not longer than ts.collateralList). However, that would not help if a collateral is revoked and a new one is added. Another solution would be to provide pairs of token addresses and amounts, which would solve the problem completely.

Assessed type

Other

QA

DiamondProxy License

DiamondProxy is a fork of https://github.com/mudgen/diamond-3/blob/master/contracts/Diamond.sol, but has changed the license from MIT to CC0-1.0. While this should be no problem legally (as far as I know), there is no reason to change the license in my opinion.

Nested Loop in Redeemer._redeem

The function _redeem iterates over all tokens. Moreover, the check LibHelpers.checkList(tokens[i], forfeitTokens) (which is linear in the length of forfeitTokens) is performed within loop iteration. Therefore if there are $n$ tokens and $m$ tokens to be forfeited, the overall complexity will be $O(n*m)$. This can be very expensive depending on the values of $n$ and $m$. As an alternative, the forfeited tokens could be stored in a mapping in the beginning and this mapping could be queried (constant complexity) within each iteration.

RewardHandler.sellRewards requires trust in seller

There is a check within RewardHandler.sellRewards to confirm that the balance of collateral assets has increased. However, this check does not change the fact that trust in the seller is required. A malicious seller could still craft an order with huge slippage that sells tokens at a very poor rate and only increases collateral slightly. An alternative would be to construct the 1inch payload dynamically instead of relying on the user to provide it.

Reliance on decimals()

LibSetters.addCollateral queries decimals() and therefore relies on the fact that tokens implement this function. While this is not guaranteed and generally against EIP 20 ("OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present."), it should not be a huge problem in practice because most ERC20 tokens implement the functions. Nevertheless, because the collateral is provided by the user anyways, an (optional) alternative would be to let him also provide the decimals.

(Temporary) manipulations of IManager.totalAssets() would break the system

One potential attack vector for the system is (temporarily) manipulating the return value of IManager.totalAssets(). Depending on how the strategies are implemented, it may be possible to inflate their values, for instance by using flash loans and temporarily depositing / withdrawing. Because no strategies were in-scope, it is not possible to judge if this is currently possible, but it is something to keep in mind (and potentially document) when building strategies.

Third Order Taylor Approximation can be imprecise

The Savings contract uses a third-order taylor approximation for approximating $(1 + i)^n$ (i.e., continuous compounding). However, this can be imprecise for large value of $ni$. For instance, consider the difference when the assets are 10,000, $i = 0.01$ , $n = 10$:

10000*(1.01)^{10} - 10000*(1+0.01*10+10*9*0.01^2/2+10*9*8*0.01^3/6) = 0.02

Now, consider the difference for $i = 0.000001$ and $n=864000$ (one week when $n$ denotes seconds):

10000*(1 + 0.000001)^{864000} - 10000*(1+0.000001*864000+864000*863999*0.000001^2/2+864000*863999*863998*0.000001^3/6) = 278.2

An alternative would be to use a library (which usually contains higher-order terms) for the exponentiation.

System may use prices that never existed

Becuase LibOracle.read combines multiple Chainlink feeds with a different timestamp, it may use a price for an asset that has never existed (when it multiplies prices with timestamp $X$ and $Y$). This has for instance happened to Y2K in the past. It is quite hard to avoid (one could keep multiple prices and try to find a "closest match" based on the history, but this is also not perfect), but something to keep in mind.

Grifting possibilities for strategies

LibSetters.revokeCollateral performs the following check for managed collateral:

(, uint256 totalValue) = LibManager.totalAssets(collatInfo.managerData.config);
if (totalValue > 0) revert ManagerHasAssets();

Depending on the strategy used, there may be ways where another user can increase totalAssets (e.g., donating 1 wei), making the removal of a collateral impossible.

Collateral may not be revokable because of rounding

LibSetters.revokeCollateral performs the following check:

if (collatInfo.decimals == 0 || collatInfo.normalizedStables > 0) revert NotCollateral();

However, because of the rounding within _swap, it can happen that normalizedStables is a very small value (e.g., 1 wei) and it is not possible to decrease it further.

Swapper assumes 18 decimals for the stable coin

The Swapper contract assumes in multiple places that the stable coin contract has 18 decimals (e.g., here). While this is true for AgToken's, the system is intended to be general and usable with other underlying stablecoins according to the whitepaper. Therefore, making this configurable would make sense in my opinion.

Overall, I really like the design and the implementation of the system. The USDC depeg has shown that we need to integrate stablecoins with resilience in mind and should not simply rely on the fact that the peg holds (either explicitly by assuming a 1:1 exchange rate or implicitly by building systems that enable arbitrage opportunities when a depeg happens). Transmuter provides a very nice solution to this problem. I think it is nice that the design disincentives redemptions when the collaterization ratio is slightly below 100%, which usually results in a lot of panic, but may be only transient. The usage of the normalizer variable is also a good optimization idea that reduces gas usage significantly.

Centralization

A user of the system has to fully trust the governance address (which could be an arbitrary EOA in principle, but will presumably be a governance system). There are many sensitive actions that could be abused by the governance address to steal funds or brick the system. Some examples are:

• Redeemer.updateNormalizer: Setting the normalizer to a value that results in much less (or more) assets when redeeming.
• SettersGovernor.recoverERC20: Redeeming collateral
• Distributor.recoverERC20: Redeeming tokens in the distributor.

This is not necessarily a bad thing and it is understandable why this functions are provided. However, it is something that users should be aware of. Moreover, it is important that the governance system is designed such that no governance attacks are possible. For instance, if someone could acquire a majority of the governance tokens for $10M and steal $20M like that, this would be a huge risk for the protocol.

Generalisability

The system can be used for arbitrary projects / stablecoins in principle, which is also a design goal according to the white paper. However, there are a few places where the implementation is slightly Angle-specific. For instance, the IAgToken interface is used for stable coin operations. While this is no huge problem, the system would be even more generalizable if it would use a more general stable coin interface or an adapter system such that other projects can write their own adapters.

Manager Contracts

Something to keep in mind is that manager contracts (which were out of scope for this audit) can have negative security implications for the system. For instance, if a manager contract does not invest / release funds properly, they may be locked up forever. Therefore, only because the main transmuter contracts are audited, does not mean that any concrete deployment with custom manager contracts is safe / audited.

Time spent:

20 hours