Potential Bug in ManualApprovalTransferManager

[xAffinity]

Hi,

I was looking through the ManualApprovalTransferManager code and was just wondering if

polymath-core/contracts/modules/TransferManager/ManualApprovalTransferManager.sol

Lines 98 to 103 in 12b1680

	if ((manualApprovals[_from][_to].expiryTime >= now) && (manualApprovals[_from][_to].allowance >= _amount)) {
	if (_isTransfer) {
	manualApprovals[_from][_to].allowance = manualApprovals[_from][_to].allowance.sub(_amount);
	}
	return Result.VALID;
	}

should return Result.ForceValid instead of Result.Valid.

polymath-core/contracts/modules/TransferManager/ManualApprovalTransferManager.sol

Line 82 in 12b1680

/** @notice Used to verify the transfer transaction and allow a manually approved transqaction to bypass other restrictions

The above implies that MATM should bypass other restrictions, and this applies for the approval case as well.

At the current state, since in

polymath-core/contracts/tokens/SecurityToken.sol

Line 629 in 12b1680

return unarchived ? (isForceValid ? true : (isInvalid ? false : isValid)) : true;

If another module returns Result.INVALID, then isInvalid will be true, causing verifyTransfer to return false and hence cause the transaction to fail.

This defeats the purpose of MATM which suppose to bypass all other restrictions and transfer modules for both the block and approvals of transactions.

Thanks!

[adamdossa]

Hey @xAffinity - thanks for looking at this.

This is by design - the MATM is allowed to override TransferManagers that return NA (e.g. the GeneralTransferManager if addresses are not on it's list), but not override a TM that returns INVALID (e.g. if an address has traded over its volume restriction in the VolumeRestrictionTM).

For transfers that really need to be forced through, regardless of any restrictions, the issuer can use the forceTransfer method directly.