Compromised owner is capable of draining all user's fund after user gives token allowance to TransferProxy.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/scripts/deployments/Deploy.sol#L377
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/TransferProxy.sol#L33

Vulnerability details

Impact

Compromised owner is capable of draining all user's fund after user gives token allowance to TransferProxy.sol

Proof of Concept

The TransferProxy.sol is important because it helps moving the fund around in AuctionHouse, AstariaRouter and in LienToken.

As we can see, this function below is powerful. It is likely that user will give the max token allowance to the contract TransferProxy, otherwise, transaction would revert in AuctionHouse.sol, LienToken and in AuctionHouse.

  function tokenTransferFrom(
    address token,
    address from,
    address to,
    uint256 amount
  ) external requiresAuth {
    ERC20(token).safeTransferFrom(from, to, amount);
  }

Well, note that the requiresAuth modifier is used in the function tokenTransferFrom, this access control model means that only specific address set up by admin can call this function.

If the admin is compromised, the admin can authorize malicious contract that can drain all the token fund from user by calling the above tokenTransferFrom after user gives token allowance to TransferProxy.sol

Because the requiresAuth modifier calls:

modifier requiresAuth() virtual {
require(isAuthorized(msg.sender, msg.sig), "UNAUTHORIZED");

_;
}

which calls:

  function isAuthorized(address user, bytes4 functionSig)
    internal
    view
    virtual
    returns (bool)
  {
    AuthStorage storage s = _getAuthSlot();
    Authority auth = s.authority; // Memoizing authority saves us a warm SLOAD, around 100 gas.

    // Checking if the caller is the owner only after calling the authority saves gas in most cases, but be
    // aware that this makes protected functions uncallable even to the owner if the authority is out of order.
    return
      (address(auth) != address(0) &&
        auth.canCall(user, address(this), functionSig)) || user == s.owner;
  }

The auth.canCall is called in MultiRolesAuthority.sol, as shown in the Deploy.sol script

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/scripts/deployments/Deploy.sol#L118

address auth = testModeDisabled ? msg.sender : address(this);
MRA = new MultiRolesAuthority(auth, Authority(address(0)));

And the relevent authorization is granted by calling:

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/scripts/deployments/Deploy.sol#L377

MRA.setRoleCapability(
  uint8(UserRoles.ASTARIA_ROUTER),
  TRANSFER_PROXY.tokenTransferFrom.selector,
  true
);

and

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/scripts/deployments/Deploy.sol#L410

MRA.setUserRole(
  address(ASTARIA_ROUTER),
  uint8(UserRoles.ASTARIA_ROUTER),
  true
);

by calling:

function setRoleCapability(
	uint8 role,
	bytes4 functionSig,
	bool enabled
) public virtual requiresAuth {
	if (enabled) {
		getRolesWithCapability[functionSig] |= bytes32(1 << role);
	} else {
		getRolesWithCapability[functionSig] &= ~bytes32(1 << role);
	}

	emit RoleCapabilityUpdated(role, functionSig, enabled);
}

A compromised admin can call:

MRA.setRoleCapability(
  uint8(UserRoles.MALICIOUS),
  TRANSFER_PROXY.tokenTransferFrom.selector,
  true
);

and

MRA.setUserRole(
  address(malicious_contract_or_account),
  uint8(UserRoles.MALICIOUS),
  true
);

Then the malicious_contract_or_account address has permission to call tokenTransferFrom, which drains user token after user gives token allowance to TransferProxy.sol. ALl he needs to do is that set the token to token that he wants to transfer and steal, the address from is victim's address, the address to is the recipient (hacker's address), the amount is how much he wants to transfer and drain.

function tokenTransferFrom(
address token,
address from,
address to,
uint256 amount
) external requiresAuth {
	ERC20(token).safeTransferFrom(from, to, amount);
}

The reference (relevant )code for MultiRolesAuthority in solmate:

https://github.com/transmissions11/solmate/blob/main/src/test/MultiRolesAuthority.t.sol

Tools Used

Manual review

Recommended Mitigation Steps

Only use safeIncreaseAllowance to give minimum approval to move the fund around, use multisig to safeguard to admin address for

[c4-sponsor]

androolloyd marked the issue as sponsor acknowledged

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

Keeping this issue as medium as what is interesting here is the possibility for the admin to exploit approvals, and not only funds holds by the contracts

[Picodes]

After this discussion (#617), I will downgrade this issue to Low severity, as I finally don't think we can consider this a case of "privilege escalation" versus changing the transfer proxy address for example. It boils down to Admin Privilege which is OOS per the automated report.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)