TreasuryManager.sol

[CRITICAL] ManagerOwner cannot transfer approved token

The tests and code leads us to believe that a managerOwner can always call deposit against a token when one of two criterea are met:

1. The managerOwner holds the FlaunchToken
2. The FlaunchToken has been approved against the TreasuryManager by an external address, and the managerOwner calls deposit

However, it looks like due to an error in the test suite for TreasuryManagerTest both the creator and owner are defined as the same address, even though the code commenting explicitly states they are different.

/// Define some useful testing addresses
address payable internal owner = payable(address(0x123));

// ..

// Flaunch a token as a third party, non-approved creator
address creator = address(0x123);

After updating this code to change the address of the creator, we can see that the test now fails:

Failing tests:
Encountered 1 failing test in test/treasury/managers/TreasuryManager.t.sol:TreasuryManagerTest
[FAIL: TransferFromIncorrectOwner()] test_Owner_CanDepositApprovedTokenWithAnyPermissions() (gas: 1304311)

Recommended Solution

To ensure that the FlaunchToken is transferred from the correct user, we should ensure that the _creator parameter is used, rather than msg.sender, in the TreasuryManager.deposit function.

// Transfer the token from the msg.sender to the contract
_flaunchToken.flaunch.transferFrom(_creator, address(this), _flaunchToken.tokenId);
emit TreasuryEscrowed(address(_flaunchToken.flaunch), _flaunchToken.tokenId, _creator, msg.sender);

Note: This does mean that the _creator cannot be set to a value other than the original sender of the FlaunchToken, but this seems like a responsible restriction.

[MEDIUM] IManagerPermissions implementation is not validated

When the setPermissions function is called, it immediately casts it to an IManagerPermissions interface and stores it against the group.

This allows for customisability, but if an address that did not support isValidCreator was added then it would still pass and set, but subsequently cause all deposit calls to revert.

The current implementation:

function setPermissions(address _permissions) public onlyManagerOwner {
  permissions = IManagerPermissions(_permissions);
  emit PermissionsUpdated(_permissions);
}

This issue could be rectified by the managerOwner at any time, either by providing a correct IManagerPermissions address or zero-address, but until they did it would result in all deposits reverting.

Recommended Solution

When permissions are set against the contract, it should try and call the isValidCreator function to ensure that it can be successfully called. The resulting response is not important, as we just need to ensure that the call does not revert.

error InvalidPermissionsContract();

function setPermissions(address _permissions) public onlyManagerOwner {
  if (_permissions != address(0)) {
    // Check if the function exists by trying to call it with a low-level call
    (bool success,) = _permissions.call(
      abi.encodeWithSignature("isValidCreator(address,string)", address(0), "")
    );

    // If the low-level call failed, then it is an invalid contract
    if (!success) revert InvalidPermissionsContract();
  } else {
    permissions = IManagerPermissions(address(0));
  }

  emit PermissionsUpdated(_permissions);
}

[LOW] Unreliable isValidCreator call

When the deposit function is being called, the tx.origin is validated against the chosen IManagerPermissions contract using the isValidCreator function call. By using a tx.origin value you may find issues when interacting with multisignatories and smart wallets.

Unless there is a valid reason for choosing tx.origin over msg.sender, we would recommend using msg.sender instead.

// Validate that the token can be deposited into by the creator
if (!isValidCreator(tx.origin, _data)) {
    revert InvalidCreator();
}

This is made even more apparent by the fact that the FlaunchToken is subsequently transferred from, and attributed to in the event, the msg.sender.

// Transfer the token from the msg.sender to the contract
_flaunchToken.flaunch.transferFrom(msg.sender, address(this), _flaunchToken.tokenId);
emit TreasuryEscrowed(address(_flaunchToken.flaunch), _flaunchToken.tokenId, _creator, msg.sender);

Recommended Solution

// Validate that the token can be deposited into by the creator
if (!isValidCreator(msg.sender, _data)) {
    revert InvalidCreator();
}

[INFO] Remove managerOwner logic bypass

If the managerOwner calls the deposit function on a TreasuryManager, then they will always be approved. This is beneficial as it allows for an approval -> deposit flow.

To give better control of future IManagerPermissions, we would recommend moving the managerOwner check to be inside of the permission implementation. This will give both the team and builders better granular control over their approach.

// In Closed.sol
function isValidCreator(address _creator, bytes calldata _data) external view returns (bool) {
    // Manager owner can always deposit
    if (_creator == ITreasuryManager(msg.sender).managerOwner()) {
        return true;
    }

    return false; // Closed permissions
}

// In Whitelisted.sol
function isValidCreator(address _creator, bytes calldata _data) external view returns (bool) {
    // Manager owner can always deposit
    if (_creator == ITreasuryManager(msg.sender).managerOwner()) {
        return true;
    }
    
    return _approvedCreators[msg.sender].contains(_creator);
}