Maia DAO - Ulysses - ABA's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L362-L372 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L188-L219

Vulnerability details

Summary

Ulysses branch ports implement a mechanism for managing active Port Strategies and balancing the tokens that these strategies can use. As it is, this mechanism has several issues:

1. a malicious (or over competitive) strategy can block withdraws of all other strategies by abusing the manage function in certain conditions
2. when there is a token reserve deficit, anybody can decide from which strategy to withdraw tokens from
   • this favors competitive strategies to game the system and take from other rival strategies, with the end goal to appear "more efficient" over time and have their maximum withdraw limit increased
3. when repaying tokens to account for the deficit in reserves, if a defect strategy has any outside call that can be maliciously hijacked, the entire withdraw cam be forever blocked. Ultimately leading to the removal of the strategy by the core team.
   • this can also happen with intent, in the case of a (previously not known) malicious strategy that will steal tokens and never return them
4. strategies are not required to actually give back the tokens they took and can't be realistically forced to either

Issues detail

(1) A strategy can block withdraws of all other strategies

When a strategy wishes to withdraw tokens to use for yielding, they call the BranchPort::manage function. Before sending the tokens, the function checks if there are enough tokens in the Port left afterwards, to maintain a minimum required amount

        // Cache Strategy Token Global Debt
        uint256 _strategyTokenDebt = getStrategyTokenDebt[_token];


        // Check if request would surpass the tokens minimum reserves
        if (_amount > _excessReserves(_strategyTokenDebt, _token)) revert InsufficientReserves();

and if not reverts. This can be abused as such:

• port token reserves are getting close to the minimum reserve limit
• alice is a strategy that uses a complicated lending/borrowing mechanism that looks for opportunities and requires specific amount of tokens at a time.
  • an opportunity is triggered for alice and there are enough reserves to actually do it so she initiates a prot withdraw
• bob, a competitive strategy, sees alices transaction and front-runs it with a withdraw of it's own, lowering the reserves and making alice fail her transaction
• since alice had a trigger and a specific opportunity had appear, she now has lost that and any potential gains

This attack is limited to the amount of tokens a strategy can withdraw daily, as such, is permanent, although it still can do damage.

(2) In deficit, anybody can decide from which strategy to withdraw tokens from

Port strategy deficit managing mechanism allows for anybody to extract tokens from whatever strategy they choose if there are not enough tokens to meet minimum reserve in the port. This is possible via the second variation of BranchPort::replenishReserves which is callable by anyone

     *     @dev can be called by anyone to ensure availability of service

Allowing this mechanism to exist creates a competitive ecosystem were, whenever the minimum reserve is not reached, strategies front-run one another in a bid to achieve overall performance and no yielding down-time. This type of mechanism can harm the protocol over time, resulting in gains for one strategy at the expense of others.

(3) A defect strategy that has its calls maliciously hijacked can block the entire withdraw forever

To replenishing the port reserves the functions BranchPort::replenishReserves are called, either by the strategy itself or by outside call. In both cases there is a check that enough funds have been sent otherwise it reverts

        // Get current balance of _token
        uint256 currBalance = ERC20(_token).balanceOf(address(this));


        // Withdraw tokens from startegy
        IPortStrategy(msg.sender).withdraw(address(this), _token, _amount);


        // Check if _token balance has increased by _amount
        require(ERC20(_token).balanceOf(address(this)) - currBalance == _amount, "Port Strategy Withdraw Failed");

In an ideal environment this would be enough, but considering the nature of web3, any strategy that may have say a valid IPortStrategy::withdraw implementation but somehow allows for external calls to happen, a malicious attacker can hijack flow and block this withdraw by simply sending 1 WEI back to the prot, making the require statement, as it looks for equality == revert, thus blocking any form of taking back any tokens by the port.

(4) Strategies are not required to actually give back the tokens they took and can't be realistically forced to either

Entire strategy premise is based that strategies will voluntarily send back, at one point, any and all tokens they were loaned. This is not enforced directly and any 3rd party strategy, even non-malicious ones approved through governance, can simply choose not return any of the tokens.

More context points:

(A) A strategy takes tokens from the port via BranchPort::manage under some conditions: - it can only do so within a daily limit, set when the strategy was first added (updatable).

        // Check if request would surpass the Port Strategy's daily limit
        _checkTimeLimit(_token, _amount);

and the _checkTimeLimit implementation:

- there are enough tokens available for withdrawal, while having the minimum reserve meet. 

        // Cache Strategy Token Global Debt
        uint256 _strategyTokenDebt = getStrategyTokenDebt[_token];


        // Check if request would surpass the tokens minimum reserves
        if (_amount > _excessReserves(_strategyTokenDebt, _token)) revert InsufficientReserves();

(B) Anyone can call the BranchPort::replenishReserves version that takes from a strategy regardless of its will but limited to and only to the difference needed to balance out the reverses, entire strategy debt or 0 if no debt.

Code

        // Get reserves lacking
        uint256 reservesLacking = _reservesLacking(strategyTokenDebt, _token, currBalance);


        // Cache Port Strategy Token Debt
        uint256 portStrategyTokenDebt = getPortStrategyTokenDebt[_strategy][_token];


        // Calculate amount to withdraw. The lesser of reserves lacking or Strategy Token Global Debt.
        uint256 amountToWithdraw = portStrategyTokenDebt < reservesLacking ? portStrategyTokenDebt : reservesLacking;

(C) Once a minimum reserve amount for a strategy token is chosen it cannot be changed and a token cannot be removed, only deactivated (so removing and adding it back is not possible).

Strategy tokens are added via the BranchPort::addStrategyToken where it is the only place where the minimumReservesRatio is set

        strategyTokens.push(_token);
        getMinimumTokenReserveRatio[_token] = _minimumReservesRatio;
        isStrategyToken[_token] = true;

A strategy token can afterwards be only activated and deactivated (this allows the strategy to withdraw tokens from the port).

Taking into consideration (A), (B) and (C) an example of an unwanted situation that can appear follows:

• Protocol team alice adds strategy token USDC with a minimum reserve ration of 50%; valid values are [30%, 100%)
• alice adds strategy bob through governance call. bob has no malicious code
  • for strategy bob a daily withdraw limit of 1000 USDC is allowed
• eve and charlie are other strategies that are added by alice to the protocol
• that specific branch port reaches 100,000 USDC TVL
• bob continuously withdraw 1000 USDC every day ((A)) until, for example, he has 30,000 withdrawn (the other 20,000 is split between eve and charlie) and the protocol reaches 50% minimum reserve ratio
• for economical reasons users take out their USDC from the port, making remaining TLV 30,000 (20,000 were withdrawn by users)
• at this point the reserve ratio is 37.5% (30K out of 80K) which is below 50% as such, from any strategy anyone can call for the missing 10K (to achieve 50% ratio)
• bob sees this and immediately front-runs the replenishReserves ((B)) on eve; eve is forced to pay what it can, if not enough, bob goes after charlie in the same transaction, and so forth until the minimum is reached and he is safe
• alice (protocol team) feels that the one strategy has a too big proportion of the debt so it wishes for it to return a part of the tokens
• bob does not comply, but remember, bob does not have any malicious code in the strategy, he is just using the mechanism as it is
• alice cannot now force bob to give back the tokens, she must wait until the minimum is again not reached to force take from him
• at that point, bob again takes from another strategy, by calling replenishReserves on it
• if alice actually wishes to make bob give back tokens, since she cannot change the minimum (because of (C)), deactivating USDC or the bob strategy does not help, the only alternative she has is to hope that more users take their deposits out, so that the TVL drops bellow the minimum and go through each and every strategy because bob would front-run any and all replenishReserves calls
• effectively, if alice wants bob out, she needs to dry up all strategies up to that point. This is even more difficult in a bull market since users would deposit in the system, the minimum will not be reached that easy, making the tokens bob has, the 30,000, permanently lost

Impact

Abusive strategies can whittled paying back tokens or indirectly cause harm to other strategies.

Tools Used

Manual analysis

Recommendations

To avoid any abuse of the system the following changes are recommended:

1. add a function to change the minimum reserve ratio for a strategy token for use in drastic cases and, of course, governance callable only
2. allow that anybody may call the equivalent replenishReserves but do not allow then for which strategy to take from; add a priority queue that first in to withdraw are the first to be taken from (FIFO); this way is a more fair version
3. (a bit controversial) create a function to forcibly withdraws from a given strategy all debt taken by it at up to that point, callable only by the protocol team
4. in the branch port, change the require statement within both replenishReserves versions to >= instead of ==

Assessed type

DoS