Skip to main content

SmartPy Version Migration Postmortem (Incident #1)

Date: 2022-04-28

Authors: dcale

Status: Complete, action items in progress

Summary: A SmartPy version migration has gone wrong, allowing internal entrypoint to be called externally.

Impact: All internal entrypoints could be called by an external address, effectively allowing someone performing the action in the name of the last user interacting with the contract. No funds were directly at risk at any time during the incident.

Root Causes: SmartPy deprecated @sp.sub_entry_point and @sp.global_lambda in favor of a parametrized @sp.private_lambda. During the migration to the new system our verify_internal lambda was configured to use the parameter wrap_call=False. Since verify_internal was only verifying whether the caller of the entrypoint was the contract itself and not "eventful" we could see the lambda in the transpiled Michelson but it was never called. Only wrap_call=True will force the lambda even if not "eventful" to be computed at the defined point.

EngineAddresswrap_call=False
ubtc tezKT1VjQoL5QvyZtm9m1voQKNTNcQLi5QiGsRZOK
ubtc tzbtc LBKT1NFWUqr9xNvVsz2LXCPef1eRcexJz5Q2MHOK
uusd tezKT1FFE2LC5JpVakVjHm5mM36QVp2p3ZzH4hHOK
uusd tzbtcKT1VwywA1XM7PL3UVLXA8vmDPvLsGHwgS7wSNOK
uusd tzbtc LBKT1FzcHaNhmpdYPNTgfb8frYXx7B5pvVyowuOK
udefi uusdKT1B2GSe47rcMCZTRk294havTpyJ36JbgdeBOK
udefi tezKT1ALVxK1YPsf1JfyqfivZT3rGCPwvebFZjsNOK
udefi tzbtc LBKT1U3RkwL3r7wi7tdjFxkjfDGfPrKrYFYGFhNOK

Trigger: A manual test routine has shown the unexpected behaviour.

Resolution: The engines have been removed from the list of admin allowing them to mint and burn synthetic assets, as well as the ability to set a YOU weight. New engines with the fix (basically setting wrap_call=True) have been deployed.

Detection: A user in discord complained about not being able to convert.

Action Items:

Action ItemTypeOwnerState
Identify engines with flawmitigatedcaleDONE
Remove the engines from the admin list of the synthetic asset in a multisig ceremonymitigateyouves signersDONE
Fix the engine code with the findingpreventflorinDONE
Add internal entrypoint calls unit testspreventflorinDONE
Add internal entrypoint calls to deployment testing protocolpreventflorinDONE
Redeploy the new enginespreventflorinDONE

Lessons Learned#

What went well#

  • As soon as the team was made aware of the issue, the mitigation was implemented and deployed within less than 1 hour 30 minutes (including the multisig ceremony).
  • The youves multisig parties understood the urgency and reacted on this unplanned issue within 5 minutes. (thank you!!!)

What went wrong#

  • Deployment testing procotol did not check the internal entrypoints.

Where we got lucky#

  • Unplanned manual test decided to also test the "internal" entrypoints.
  • set_target_price checks the sender.

Conclusion#

  • Also when it seems unneccessary the additional check in set_target_price made sure that no unlimited amounts of synthetic assets could be minted.
  • Manual test protocols are important and need to cover every entrypoint, also if the expected behaviour is a fail.

Timeline#

2022-04-28 (all times UTC)

  • 11:40 manual test identifies weird behaviour
  • 11:55 engines with behaviour listed in list
  • 12:00 preparation of multisig execution payload
  • 12:47 multisig ceremony is kicked-off
  • 12:51 required signatures are collected
  • 13:15 flawed youves engines removed from admin list (ooRPfrFBUdKBkxRoqDvJnu8zUxVywRtTkjQkXBpYbZs1vuwU69i)