The Cheddar DAO [cheddar.sputnik-dao.near] was working to establish an LP from the Skyward sale and had a proposal to transfer 1M Cheddar to its account on Ref Finance. However, the wrong function call was inadvertently issued ft_transfer was called instead of ft_transfer_call.
This means that the 1M Cheddar was directly transferred to the Ref contract [v2.ref-finance.near], instead of being deposited to the cheddar.sputnik-dao.near user account on Ref.
Actually, this is a common issue we would see on defi contract with NEP-141 standards. As contract can only aware of token deposit when it is transfered into with ft_transfer_call, A simple transfer (ft_transfer) would cause token locked in the defi contract account, especially the contract is locked (all key removed).
Hey Marco,
I support proposal #118 (upgrading the contract to have an admin withdraw function) but I think the changes should be reverted as soon as the tokens are rescued.
Having a function with that power on the contract can be dangerous especially that
the exchange contract is constantly evolving
Sputnik contracts aren’t well battle-tested yet (they’re currently undergoing an audit).
I know there are other equally powerful functions (like upgrade) but I think we want to minimize their risk (example: add time-lock on upgrades) and especially don’t want to have more of these overly powerful function.
What do you think about reverting the changes after the rescue, or adding a time-lock?
I understand your concern about the safety pb behind that feature. But I don’t think the revert solution is a good way to address that. Upgrading contract is the action we take the most risk in all DAO operations. Cause the changes it does are not limited and difficult to verify for now. So to alleviate a risk using a more risky way seems not good. On the other hand, if the DAO can no longer be trusted, Most NEAR Eco projects would collapse, It is hard to survive it by just closing some interfaces from the DAO.
FYI: The mistaken transfer action uses ft_transfer which our ref-contract won’t aware of. So retrieve them back to refDAO won’t cause any issue on contract side.
Thanks for the explanation. I see that reverting contract changes isn’t favorable.
What about time-locks ? It doesn’t have to be a long duration, something like 24hrs is good enough to react on security reports, and also enough to check all incoming contract change requests. A bot can be set up to regularly monitor pending changes (exp: poll each hour for pending change requests).
Interesting suggestion. We can discuss it further in deep on tg I guess. And FYI, of the purpose of the time-locks, we already have several alternative ways to take care.
We have code testing and review by experts each time the contract code upgrade,
We have integration tests on testnet for the new version and upgrade action,
After deployment on mainnet, we have guardians that will halt the whole contract in emergency.
I agree that internal team security processes are of utmost importance, and I’m proud of the effort made by you guys in that regard.
But my suggestion is about protecting against an attacker that gained owner privileges on the contract. Even if no such bug exists now, the contract is upgradeable and we never know what bugs might be introduced in the future, and slip through the test + preview process. Such attacker can instantly make lots of damage: call the owner’s token withdraw, kick out guardians, change the owner, upgrade etc…
With the presence of time locks on all owner functions these interactions would be delayed by an amount of time (like 24hrs), giving time to Ref’s core team to react accordingly.
Imho the only privileged function that shouldn’t get a time-lock is pausing the contract, and refusing proposed changes.
My technical understanding is limited, but I am taking the issues raised by @chluff very seriously and carefully following the conversation and explanations provided by @Marco
Few comments and suggestions:
Would introducing the ability to upgrade the contract mean that we lose the ‘audited’ status on the current contract?
How would we rate the Contract Upgradeability risk on a scale from 1-10?
How would we rate the Sputnik Contracts risk on a scale from 1-10? (in reference to reverting the upgradeability upgrades).
How urgent is the return of the funds needed by Cheddar Finance?
To be honest, given some of the risks and uncertainties outlines above, it doesn’t seem to me reasonable or appropriate at this point to make significant changes to the REF contract which could introduce risks. I would propose delaying this measure as long as reasonably possible until we can be reassured that all the risks outlined above have been properly mitigated.
I think the fact is eventually we would go into the situation that must upgrade a contract. It maybe a major function upgrade, maybe a bug fix, maybe a neccessary compensation of current functions like this cheddar issue.
It is not how urgent the cheddar need their token back, but shall we take care for that kind of user misoperation. Shall we just stand there and do nothing? If not, I guess we have to figure out a way to return their token back, that is what unexpected in current production logic. So it is a compensation for current functions.
Finally, it turns out to be this problem: how to ensure the upgraded code is safe. I think at this point, @AVB 's concern really makes sense. So, as you may not know, we have some mechanism for that:
Now we have inner audtition from at least 2 experts each time we upgrade the contract.
We have dedicate testnet tests for each changes we made;
The thridparty firm audition package includes some possible future upgrade.
And, to this cheddar thing, the modification is not so significant as it looks like.