Add Value.currencySymbolValueOf

[zliu41]

I noticed that this is used by some scripts and they implement it themselves with the less efficient PlutusTx.sum . Map.elems.

[ana-pantilie]

LGTM, but you might want someone with better knowledge of the codebase to approve as well.

[effectfully]

LGTM

[effectfully]

Are you sure it doesn't get inlined away? Would be good to add the golden test first, then redefine the function. Anyway, not important.

[zliu41]

Yes, with sum . Map.elems:

-({cpu: 26943308
-| mem: 104124})

+({cpu: 30347308
+| mem: 118924})

And it is not supposed to be inlined away in the first place.

[effectfully]

Thanks!

And it is not supposed to be inlined away in the first place.

Yeah, we're not doing fusion/deforestation, in Haskell it could optimize away.

[effectfully]

Perhaps we could have Map.foldr? Maybe not worth it.

[zliu41]

Probably not.

[colll78]

@zliu41 @effectfully

Please add very explicit documentation around this function. It is extremely dangerous. This function is perhaps responsible for more critical vulnerabilities than any other utility function in onchain code. I personally have discovered a number of vulnerabilities that allow for malicious actors to completely drain protocol liquidity in very high profile widely used open-source libraries and DApp codebases.

See for instance:
Liqwid-Labs/agora#202

The issue is due to the fact that developers tend to intuitively use this function in minting policies in order to enforce that only one token is minted, because they believe that it returns the number of tokens with the given currency symbol in a value. The issue is symbolValueOf does not enforce that only one token name of the cs is present, nor that all the entries in the value are positive or negative. Consider the following:

ourMintingPolicy :: ScriptContext -> BuiltinUnit 
outMintingPolicy ctx = 
  case (scriptContextRedeemer ctx) of 
    -- minting conditions (ie deposit liquidity for LP tokens)
    Mint -> ... 
    -- developer expects that this check will enforce that this redeemer can only be used to burn a token
   -- and minting will be impossible
    Burn -> symbolValueOf mintedValue ownCS == -1 
  where 
    (MintingScript ownCS) = (scriptContextScriptInfo ctx)
    mintedValue = txInfoMint (scriptContextTxInfo ctx)

The issue with the above use is that a malicious actor with two such tokens could perform a transaction using the Burn redeemer that burns two tokens and mints one and the validation will succeed. For instance, if they set txInfoMint = PMap[(PCurrencySymbol "",PMap [(PTokenName "",0)])]), (PCurrencySymbol ourCS, PMap [(PTokenName "TokenA", -2), (PTokenName "TokenB", 1)])])]

Instead of a non-specific symbolValueOf function, it is typically better to define two variants:
positiveSymbolValueOf and negativeSymbolValueOf. This forces the developer to be more deliberate in their use and thus reduces the risk of such vulnerabilities making it into production.

Here is a (completely unoptimized) reference implementation of such functions:

psymbolValueOfHelper ::
  forall
    (keys :: KeyGuarantees)
    (amounts :: AmountGuarantees)
    (s :: S).
  Term
    s
    ( (PInteger :--> PBool)
        :--> PCurrencySymbol
        :--> ( PValue keys amounts
                :--> PInteger
             )
    )
psymbolValueOfHelper =
  phoistAcyclic $
    plam $ \cond sym value'' -> unTermCont $ do
      PValue value' <- pmatchC value''
      PMap value <- pmatchC value'
      m' <-
        tcexpectJust
          0
          ( plookupAssoc
              # pfstBuiltin
              # psndBuiltin
              # pdata sym
              # value
          )
      PMap m <- pmatchC (pfromData m')
      pure $
        pfoldr
          # plam
            ( \x v ->
                plet (pfromData $ psndBuiltin # x) $ \q ->
                  pif
                    (cond # q)
                    (q + v)
                    v
            )
          # 0
          # m

-- | Sum of total positive amounts in Value for a given policyId
ppositiveSymbolValueOf ::
  forall
    (keys :: KeyGuarantees)
    (amounts :: AmountGuarantees)
    (s :: S).
  Term s (PCurrencySymbol :--> (PValue keys amounts :--> PInteger))
ppositiveSymbolValueOf = phoistAcyclic $ psymbolValueOfHelper #$ plam (0 #<)

-- | Sum of total negative amounts in Value for a given policyId
pnegativeSymbolValueOf ::
  forall
    (keys :: KeyGuarantees)
    (amounts :: AmountGuarantees)
    (s :: S).
  Term s (PCurrencySymbol :--> (PValue keys amounts :--> PInteger))
pnegativeSymbolValueOf = phoistAcyclic $ psymbolValueOfHelper #$ plam (#< 0)

[zliu41]

See for instance:

The linked issue was in 2022 and currencySymbolValueOf was added this year. How could currencySymbolValueOf have been the cause?

The issue is symbolValueOf does not enforce that only one token name of the cs is present

That should be obvious - there's no reason to believe that only one token is present. The whole point of currencySymbolValueOf is to calculate the total value across all tokens, so there certainly can be more than one. The Haddock also says "Get the total value of ...".

nor that all the entries in the value are positive or negative.

This is less obvious; we can add the following - "Each token of the currency symbol may have a value that is positive, zero or negative". Is that better?

Having said that, if one is not aware of this fact, then even without currencySymbolValueOf, they'd probably make the same mistake by writing something like
case Map.lookup cur mp of Just tokens -> sum (Map.elems tokens), which would do the same thing. currencySymbolValueOf is just a helper function that does that.

[colll78]

The linked issue (and most such exploits) are due to the Plutarch equivalent of this function which is called pcurrencySymbolValueOf. Yes I agree that the token name part is obvious.

This is less obvious; we can add the following - "Each token of the currency symbol may have a value that is positive, zero or negative". Is that better?

Yes this is perfect.

Having said that, if one is not aware of this fact, then even without currencySymbolValueOf, they'd probably make the same mistake by writing something like case Map.lookup cur mp of Just tokens -> sum (Map.elems tokens)

The difference is in this case the developer would have a much higher likelihood of realizing that there is an issue with such a use because by implementing it they are forced to understand how it works, specifically that it does not count the number of tokens (positive) with a given currency symbol, instead it sums the quantities of all tokens with a given currency symbol. Also this is a strong reason for why the value in txInfoMint should have a different type than Value (ie. MintedValue) because it does not obey the invariants that all other Value's have (ie no negative quantities). In all other values, currencySymbolValueOf can safely be used because there is no need to qualify for positive or negative entries.

[zliu41]

Ok, I added the sentence to the Haddock, and opened #6445 in the backlog. Thanks for bringing this to our attention.

[colll78]

Since I last mentioned it, I have discovered the same vulnerability resulting from misunderstanding of symbolValueOf (and equivalents in other languages) in two additional protocols:
input-output-hk/partner-chains-smart-contracts#134
mlabs-haskell/dens-onchain#1

For some reason, this function is responsible for critical vulnerabilities at an unbelievably high rate.

[effectfully]

@zliu41 sounds like we should have a separate issue for discussions around symbolValueOf?

[zliu41]

@zliu41 sounds like we should have a separate issue for discussions around symbolValueOf?

That's what #6445 is for, unless we are talking about a different problem.

[colll78]

i don’t think documentation is sufficient for this. I would suggest removing this and replacing it with a variant that counts only positive quantities, and a separate variant that counts only negative quantities. I spend a lot of time reviewing open-source dApps and I have never encountered a single safe use of this function or variants of it in other languages. Every time I encounter it, the use results in the same exploit.

The vulnerabilities that arise from misusing this on minted values often allow draining the associated protocol of all funds.

[effectfully]

@zliu41 I'm not really following this discussion, but can we move it to the issue that you referenced or some other issue? Maybe by giving a summary there or at least providing a link and continuing there. It would be unfortunate to keep all that context in a merged PR.

[zliu41]

i don’t think documentation is sufficient for this. I would suggest removing this and replacing it with a variant that counts only positive quantities, and a separate variant that counts only negative quantities.

I think this is covered by #6445, which is about making a newtype that represents values with non-negative quantities.

I'm not really following this discussion, but can we move it to the issue that you referenced or some other issue?

Can you be more specific? #6445 does link to this issue in its description, so I'm not sure what else to do.

[colll78]

ussion, but can we move it to the issue that you referenced or some other issue? Maybe by giving a summary there or at least providing a link and continuing there. It would be unfortunate to keep all that context in a merged PR.

Ah you are correct, that's exactly what that is for, and it should address this issue! Sorry for the misunderstanding.