search

ton-blockchain / multisig-contract-v2

Multiowner wallet
54 stars 19 forks source link

Auditing report - Alex Telegin #46

Closed drinkius closed 6 months ago

drinkius commented 6 months ago

In this issue I would like to document my findings, was late to discover the competition and given only 7-day opening period - might not have enough time to provide PoCs for every issue, would expand the issue with comments until the deadline. Some of the issues might be an intuition at this point, and further research might reveal that it's not actually a vulnerability (also keeping these to a minimum not to spam, but I'll document some for now anyway)

General comments

1. Compiled code in Multisig.compiled.json is outdated

Given the code in commit 107ee13a - after compilation the output in build folder changes. This might lead to bad deployments and might confuse beginner developers. The code needs updating to ensure consistency and avoid potential loss of funds if the compiled version contains hidden vulnarabilities

2. Gas savings in recv_internal

There are two places where recv_internal is implemented with extra variable balance and it's advisable to skip unused to save gas: https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/multisig.func#L84 https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/order.func#L161

3. Scripts for order signing and general protocol interactions

Currently there is a limited set of scripts to interact with multisig deployments in the main branch. Partially this is fixed in the scripts branch but, for example, the signing option was missing still. Since multisig is a complex set of contracts - it's better to be able to test the functionality on chain and testnet, thus it's better to include such scripts in the future to get the most out of auditing contests, instead of letting every developer from outside the TON ecosystem to figure everything out themselves. I've added the basic scripts in my branch here, however it's worth investing time in better implementation in the future

drinkius commented 6 months ago

Intuition

Reliance on signer's index to verify the signature can be abused (possibly by changing index?)

While verifying the signature on second init of order: https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/order.func#L222

The contract only relies on the index of signer which is passed from the caller. Giver current restrictions it can be only the multisig, so abuse vector would be to make multisig pass an init with an index that doesn't belong to the current caller. This can be done as part of the nested calls, probably during the execution of another order which has necessary calls to this other order that's going to be attacked. The entrypoint would be the op::new_order flow since we try to abuse the system by using only one signer's access to the contract bypassing the rest of the signers

EmelyanenkoK commented 6 months ago

It seems that since we check that signers on second approve_on_init is the same as in the storage https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/order.func#L214 index manipulation is impossible.

drinkius commented 6 months ago

It seems that since we check that signers on second approve_on_init is the same as in the storage

https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/order.func#L214

index manipulation is impossible.

Seems so, only if there are some issues introdiced with hashing function implementation update for dicts and it doesn't account for the order of elements inside