osmosis-labs / osmosis

The AMM Laboratory
https://app.osmosis.zone
Apache License 2.0
889 stars 593 forks source link

Design analysis of BeforeSend hook implementation in x/TokenFactory #2567

Open dalmirel opened 2 years ago

dalmirel commented 2 years ago

During the audit of the possible BeforeSend hook impact, the Implementation of the hook is analyzed for issues and improvements.

Artifact:

Context in which audit took place **TokenFactory BeforeSend hook implementation facts:** - hook implementation should call the sudo execution of the BeforeSend entry point FOR EACH token factory denom being sent - cosmwasm smart contract is registered as a hook for a specific token factory created denom - the smart contract should implement sudo BeforeSend mesage entry point - this expectation will be proposed as a change in cw20 standard **Expectations:** - execution of the smart contract's BeforeSend msg will return an error upon some criteria and freeze the sending of a certain type of token factory denom - the smart contract should work only with the minimum set of the data

Analysis Summary:

The current design for SudoMsg BeforeSend, from a smart contract example:

pub enum SudoMsg {
    BeforeSend {
        from: String,
        to: String,
        amount: Vec<Coin>,
    },
}

Sudo execution from BeforeSend hook implementation is called per token factory denom. https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/tokenfactory/keeper/beforesend.go#L82-L85 . https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/tokenfactory/keeper/beforesend.go#L105 while types.SudoMsg creation: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/tokenfactory/keeper/beforesend.go#L90-L96 contains all the coin info being sent.

Suggestion: Change the BeforeSend Msg to contain only wasmvmtypes.Coin info, created with: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/tokenfactory/keeper/beforesend.go#L58 and propose the cw20 standard change accordingly:

pub enum SudoMsg {
    BeforeSend {
        from: String,
        to: String,
        amount: Coin,
    },
}
dalmirel commented 2 years ago

Tagging audit collaboration team, to review issues as agreed. @ValarDragon @sunnya97