search

near-daos / sputnik-dao-contract

Smart contracts for https://app.astrodao.com
https://astrodao.com/
MIT License
108 stars 76 forks source link

Add assert check on update method inside the factory #136

Closed ctindogaru closed 2 years ago

ctindogaru commented 2 years ago

Currently any DAO can update any DAO, which is a big security concern.

Limit the update process only to the owner of that DAO or the factory owner.

TrevorJTClarke commented 2 years ago

@ctindogaru thanks for the update.

Currently however, the upgrade isn't possible with this method due to: https://github.com/near-daos/sputnik-dao-contract/blob/518ad1d97614fff4b945aba75b6c8bd2483187a2/sputnikdao-factory2/src/factory_manager.rs#L96 Which doesnt pay for storage, so essentially this method is broken. Once that line gets changed, it will then be a problem. Do you mind fixing the attached payment here as well?

ctindogaru commented 2 years ago

I think that's a separate issue, and I would open a separate PR for it. Would you mind merging this one and open a new one for the deposit issue?

ctindogaru commented 2 years ago

@ctindogaru thanks for the update.

Currently however, the upgrade isn't possible with this method due to:

https://github.com/near-daos/sputnik-dao-contract/blob/518ad1d97614fff4b945aba75b6c8bd2483187a2/sputnikdao-factory2/src/factory_manager.rs#L96

Which doesnt pay for storage, so essentially this method is broken. Once that line gets changed, it will then be a problem. Do you mind fixing the attached payment here as well?

I m not sure why do you need deposit here?

TrevorJTClarke commented 2 years ago

I m not sure why do you need deposit here?

Without the deposit - the method fails, as it checks for payment to cover storage in "store_blob", look for ERR_NOT_ENOUGH_DEPOSIT.

TrevorJTClarke commented 2 years ago

Did this as PR: https://github.com/near-daos/sputnik-dao-contract/pull/138 so i could merge while you sleep :)

ctindogaru commented 2 years ago

I'm not seeing any difference between #138 which got merged and this PR?

ctindogaru commented 2 years ago

I m not sure why do you need deposit here?

Without the deposit - the method fails, as it checks for payment to cover storage in "store_blob", look for ERR_NOT_ENOUGH_DEPOSIT.

store_blob is not involved in any way in this flow.

This flow goes like this: factory::update -> dao::update -> dao::migrate

No deposit is needed in any of these methods, so nothing should fail.