Angle Protocol - Invitational - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

A disputer loses their deposited dispute tokens if someone disputes the tree after them.

Proof of Concept

The Distributor.disputeTree function is used to dispute a Merkle tree. The function requires the caller to deposit disputeAmount of disputeToken; the caller address is stored in the disputer state variable.

When a dispute is resolved by the governor/guardian via a call to Distributor.resolveDispute, the deposited funds are return to the disputer if the dispute is recognized as valid by the governor/guardian.

Since the Distributor.disputeTree function is not restricted (a tree can be disputed by anyone), it's likely that when an invalid/malicious tree is submitted, there will be multiple parties willing to dispute it (e.g. the distribution creator and some of the reward claimants). However, any subsequent call to disputeTree will override the disputer address, and thus the previous disputer won't be able to get their deposited tokens back after the resolution of the dispute.

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

diff --git a/contracts/Distributor.sol b/contracts/Distributor.sol
index bc4e49f..df56f5f 100644
--- a/contracts/Distributor.sol
+++ b/contracts/Distributor.sol
@@ -231,7 +231,8 @@ contract Distributor is UUPSHelper {
     /// @notice Freezes the Merkle tree update until the dispute is resolved
     /// @dev Requires a deposit of `disputeToken` that'll be slashed if the dispute is not accepted
     /// @dev It is only possible to create a dispute for `disputePeriod` after each tree update
     function disputeTree(string memory reason) external {
+        if (disputer != address(0)) revert UnresolvedDispute();
         if (block.timestamp >= endOfDisputePeriod) revert InvalidDispute();
         IERC20(disputeToken).safeTransferFrom(msg.sender, address(this), disputeAmount);
         disputer = msg.sender;

Assessed type

Other

Lines of code

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

Vulnerability details

Targets

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

Impact

Users can claim rewards from a Merkle tree that's being disputed. This can potentially lead to loss of funds since a malicious trusted EOA can claim funds from a malicious tree while it's being disputed.

Proof of Concept

The Distribution.getMerkleRoot function is used to get the current Merkle root during claiming. The function is aware of the dispute period of the current root and returns the previous root if the current tree is still in the dispute period.

However, the function doesn't take into account the situation when:

1. a tree was disputed (i.e. the disputer address is set);
2. and the dispute period has finished (i.e. when block.timestamp >= endOfDisputePeriod).

Such situations can happen realistically when a tree is disputed closer to the end of its dispute period and/or when the governor/guardian takes longer time to resolve the dispute. In such situations, the dispute period checks in the above functions will pass, however the disputer address will be set, which means that the tree is being disputed and shouldn't be used in claims.

As an example exploit scenario, a malicious trusted EOA can add a Merkle tree root that lets them claim the entire balance of the contract. Even if the tree gets disputed quickly, the success of the attack boils down to how quickly the governor/guardian will resolve the dispute. To increase the chance, the attack can be deliberately executed when the governor/guardian are not active or available immediately.

Tools Used

Manual review

Recommended Mitigation Steps

When the disputer address is set (after a call to disputeTree), consider treating the current tree as disputed, no matter whether the dispute period has passed or not. E.g. consider these changes:

diff --git a/contracts/Distributor.sol b/contracts/Distributor.sol
index bc4e49f..8fb6a4c 100644
--- a/contracts/Distributor.sol
+++ b/contracts/Distributor.sol
@@ -197,7 +197,7 @@ contract Distributor is UUPSHelper {

     /// @notice Returns the MerkleRoot that is currently live for the contract
     function getMerkleRoot() public view returns (bytes32) {
-        if (block.timestamp >= endOfDisputePeriod) return tree.merkleRoot;
+        if (block.timestamp >= endOfDisputePeriod && disputer == address(0)) return tree.merkleRoot;
         else return lastTree.merkleRoot;
     }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Stablecoin holders can receive wrongly calculated yield in the SavingsVest contract. Also, wrong vesting profit can be slashed when the protocol is under-collateralized.

Proof of Concept

The SavingsVest contract lets users deposit their stablecoins and earn vested yield when the stablecoin in the Transmuter protocol is over-collateralized. The interest is accrued via calls to the SavingsVest.accrue function.

There are two parameters that affect the profit of depositors:

1. protocolSafetyFee is the fees paid to the protocol;
2. vestingPeriod is the period when the yield remains locked.

The two parameters can be changed via the setParams function. However, before they're changed, the current interest is not accrued. E.g. this may lead to:

1. If protocolSafetyFee is increased without accruing interest, the next accrual will happen at the increased fees, which will reduce the rewards for the depositors.
2. If vestingPeriod is increased without accruing interest, the yield will be locked for a longer period and the next accrual may slash more vested yield.

Thus, users can lose a portion of the yield that was earned at a lower protocol fee after the fee was increased. Likewise, increasing the vesting period may result in slashing yield that was earned before the period was increased.

Tools Used

Manual review

Recommended Mitigation Steps

In the SavingsVest.setParams function, consider accruing interest with the current parameters before setting new protocolSafetyFee and vestingPeriod.

Assessed type

Other

[L-01] Missing indexed event field

Targets

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

Impact

Querying and filtering Claimed events by username or token address will be difficult and in some cases impossible.

Proof of Concept

The Claimed event doesn't have indexed fields (Distributor.sol#L111):

event Claimed(address user, address token, uint256 amount);

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding indexed fields to the Claimed event. For example, indexing the user field will make it possible to find all tokens claimed by an address.

[L-02] answeredInRound is deprecated

Targets

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/libraries/LibOracle.sol#L152-L153

Impact

In some distant future, answeredInRound may start returning a stub value (e.g. 0), which will break the check for all cases.

Proof of Concept

As per the Chainlink documentation, the answeredInRound variable has been deprecated:

answeredInRound: Deprecated - Previously used when answers could take multiple rounds to be computed

Thus, it's no longer necessary to check that roundId > answeredInRound. In some distant future, answeredInRound may start returning a stub value (e.g. 0), which will break the check for all cases.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the roundId > answeredInRound check from the LibOracle.readChainlinkFeed function.

[L-03] Risky use of toggle functions

Targets

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/transmuter/facets/SettersGuardian.sol#L16-L18 https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/savings/Savings.sol#L220-L224

Impact

The governor and the guardian may fail to pause the Transmuter or the Savings contract when pausing is needed.

Proof of Concept

SettersGuardian.togglePause and Savings.togglePause are functions used to pause the contracts, they can only be called by the governor or the guardian. These functions are error-prone since they depend on the current state of the blockchain. If the governor and the guardian independently trigger either of the functions at around the same time (e.g. an incident may force them to pause the contract ASAP), the contract won't actually be paused since the second call will unpause it.

Tools Used

Manual review

Recommended Mitigation Steps

For the two functions, consider replacing them with functions that explicitly set the paused state to either 0 or 1.