Default Instruction implementation allows invalid CreditReceiver allocation

[kindofdev]

Hello, I think there's an issue on CreditReceiver allocation.
The way is implemented allows the following scenario:

-- 
--  INSTRUCTION:
-- 
--            bank
--  sender ---------> receiver
--
--
--  ACCOUNTS:
-- 
--              bank
--           /         \
--    sender         receiver
--
--  bank allocates with CreditReceiver
--  
--

For instance:

-- Allocate instruction
  (cashInstructionCid, _) <- submit bank do
    exerciseCmd cashInstructionCid Instruction.Allocate with
      actors = Set.singleton bank; allocation = CreditReceiver

  -- Approve instruction
  cashInstructionCid <- submit receiver do
    exerciseCmd cashInstructionCid Instruction.Approve with
      actors = Set.singleton receiver; approval = TakeDelivery receiverAccount

  -- Settle batch
  submitMultiMustFail [bank] [publicParty] do
    exerciseCmd batchCid Batch.Settle with actors = Set.singleton bank

daml-finance/src/main/daml/Daml/Finance/Settlement/Instruction.daml

Line 116 in 66495d0

CreditReceiver -> do

 CreditReceiver -> do
   allMustAuthorize $ Set.singleton routedStep.custodian
   pure allocation

I would say that the code bellow might fix the issue:

 CreditReceiver -> do
   allMustAuthorize $ Set.singleton routedStep.custodian
   mustBe this Custodian routedStep.sender
   pure allocation

[johan-da]

Hi @kindofdev,

Thank you for bringing this up! I agree that it would indeed be a nice enhancement to add the sanity check for the
CreditReceiver allocation, namely:

mustBe this Custodian routedStep.sender

This is something we can consider including in a future release.

That said, we currently don't see it as a high-priority change for the following reasons:

1. The sanity check (^) is already performed during the execution of the instruction. If an instruction is "wrongly" allocated with CreditReceiver, it can't be executed —similar to how an unallocated instruction would fail when executed.
2. Custodians are expected to know how to correctly allocate instructions. In particular, they should avoid allocating with CreditReceiver unless they are the sender in the instruction.

We appreciate your feedback and will keep this suggestion in mind for upcoming updates.

Thanks again,
Johan