Maia DAO Ecosystem - LokiThe5th's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L388

Vulnerability details

Impact

Although this appears to be two seperate bugs, this is clearly one error where the developers accidentally swapped the implementation logic for _denormalizeDecimals and _normalizeDecimals. And since the fix simply requires swapping the logic for these two functions back around, it is treated as one issue here.

In the supplied repo the functionality of _normalizeDecimals and _denormalizeDecimals is swapped. This breaks core functionality related to withdrawFromPort and depositToPort for tokens which do not have 18 decimals.

This is because withdrawals for non-18 decimal tokens will be hugely inflated (meaning that a bridge can be inadvertently drained by a user requesting a small amount), while deposits for non-18 decimal tokens into the protocol are vastly deflated, which means much less is deposited into the protocol for these tokens than intended by users.

The impact is high as it will render the protocol unable to safely accept deposits or withdrawals for non-18 decimal tokens. Loss of funds is possible, but is unlikely as the bug works both ways, meaning that depositing enough tokens into the protocol to exploit it via this issue is economically difficult. However, the impact remains high because this breaks a core feature of the protocol, the intended compatibility for non-18 decimal tokens. Due to the architecture the bug will likely persist across all chains.

Proof of Concept

Overview

In BranchBridgeAgent.sol the function _normalizeDecimals(uint256 _amount, uint8 _decimals) is expected to scale a token from a non-18 decimal format to a normalized 18 decimal format. The function can be found here. But given the logic:

    return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;

It means that for a test case of tokenA which has _decimals = 6 and an _amount = 100 * 10 ** 6 (i.e. 100 tokens):

result = 100 * (10 ** 6) / 1 * 10 ** 18 = 100 000 000 / 1 000 000 000 000 000 000 = 0.0000000001

The returned result will be 0 in Solidity, meaning that transfers that rely on _normalizeDecimals (like depositToPort) will be fed amounts that are much less than expected (or even zero).

In BranchPort.sol the function _denormalizeDecimals(uint256 _amount, uint8 _decimals) is expected to take a scaled 18-decimal amount and scale it back to a _decimals-decimal amount. The code can be found here. But given the logic:

    return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);

It means that for the same test case of 100 tokens of tokenA, which is supplied with 18 decimals, but needs to be scaled to _decimals = 6:

result = (100 * 10 ** 18) * (1 * 10 ** 18) / (10 ** 6) = (100 * 10 ** 36) / (10 ** 6) = 100 * 10 ** 30 = (1 * 10 ** 26) * (10 ** 6)

The returned result would be hugely inflated. This means functions which rely on _denormalizeDecimals, like withdrawFromPort, will be fed amounts which are much greater than expected. The high impact issue here is that a user may inadvertently drain a bridge during a call to withdrawFromPort.

Test Case PoC for _normalizeDecimals

The below code can be placed in the test file ArbitrumBranchTest.t.sol

    
    MockERC20 public tokenPoC;
    address public localPoCToken;

    function testHelperAddLocalTokenNon18DecimalsArbitrum() public {
        //Get some gas.
        hevm.deal(address(this), 1 ether);

        // Create the non-18 decimal token
        tokenPoC = new MockERC20("Proof", "POC", 6);


        //Add new localToken
        arbitrumCoreRouter.addLocalToken(address(tokenPoC));

        localPoCToken =
            RootPort(rootPort).getLocalTokenFromUnder(address(tokenPoC), rootChainId);

        console2.log("New: ", localPoCToken);
        console2.log("Balance After: ", address(coreBridgeAgent).balance);
    }

    function testDepositToPortNon18Decimals() public {
        //Set up
        testHelperAddLocalTokenNon18DecimalsArbitrum();

        //Mint Tokens
        tokenPoC.mint(address(this), 10000000 ether);

        //Approve Tokens
        tokenPoC.approve(address(localPortAddress), 100 ether);

        uint256 balanceBefore = tokenPoC.balanceOf(address(this));
        uint256 depositAmount = 100 * 10 ** 6;

        //Call deposit to port
        arbitrumMulticallBridgeAgent.depositToPort(address(tokenPoC), depositAmount);
        uint256 balanceAfter = tokenPoC.balanceOf(address(this));

        console2.log("Before: ", balanceBefore);
        console2.log("After: ", balanceAfter);

        // Because of the bug we expect that `0 tokens` will be transferred
        require(balanceBefore == balanceAfter, "PoC does not hold");

        // Retry
        balanceBefore = tokenPoC.balanceOf(address(this));
        depositAmount = 10000000 ether;

        // The below should transfer all 100 * 10 ** 18 tokens to the bridge agent
        arbitrumMulticallBridgeAgent.depositToPort(address(tokenPoC), depositAmount);
        uint256 portBalanceAfter = tokenPoC.balanceOf(address(localPortAddress));

        balanceAfter = tokenPoC.balanceOf(address(this));

        console2.log("Before: ", balanceBefore);
        console2.log("Before User: ", balanceBefore);
        console2.log("After User: ", balanceAfter);
        console2.log("After Port: ", portBalanceAfter);

        require(balanceBefore != portBalanceAfter, "PoC does not hold");
        /**
         The test shows that despite trying to deposit 10_000_000 * 10 ** 18 tokens, 
         the user only transferred 10_000_000_000_000 tokens
         */
    }

Test Case PoC for _denormalizeDecimals

The below test case can be placed below testDepositNon18Decimals:

    function testWithdrawFromPortNon18Decimals() public {
        //Deposit Tokens
        testDepositToPortNon18Decimals();

        uint256 portBalanceBefore = tokenPoC.balanceOf(address(localPortAddress));
        // In this PoC scenario we are simply trying to withdraw 1 single token
        // The result is, however, that we receive 1 000 000 000 000 tokens

        // Withdrawing 1 single token
        arbitrumMulticallBridgeAgent.withdrawFromPort(address(localPoCToken), 1);


        uint256 portBalanceAfter = tokenPoC.balanceOf(address(localPortAddress));
        uint256 balanceAfter = tokenPoC.balanceOf(address(this));

        console2.log("Before Port: ", portBalanceBefore);
        console2.log("After User: ", balanceAfter);
        console2.log("After Port: ", portBalanceAfter);

        // Despite only withdrawing 1 token, the user received 1 000 000 000 000 tokens
        require(portBalanceBefore - portBalanceAfter != 1, "PoC doesn't hold");
    }

The PoCs show two things: 1) that a user trying to deposit non-18 decimal tokens into the Port will always transfer less than expected, and in many cases 0; and 2) that a user trying to withdraw non-18 decimal tokens from the Port might receive more tokens than expected, and again, in many cases withdrawals will fail as the scaled value is too large.

Tools Used

Manual code review. Foundry. A calculator.

Recommended Mitigation Steps

Correct the functionality for _normalizeDecimals and _denormalizeDecimals by swapping their implementation logic around.

Adding tests for non-18 decimal tokens is advised as well.

Assessed type

Decimal