RBAC Permissions #12

bonedaddy · 2024-05-17T06:32:31Z

Changes

Updated storage variables
Added new QueryMsg variants + handlers + tests
Added RBAC roles + role evaluation functions + tests
Added access control check to ExecuteMsg handler

Questions

When editing an existing quota, how should the stored channel_value be handled? For now I'm copying the value when editing the quota as seen here

Notes

For all functions in query.rs I changed the function signature to accept &dyn Storage trait instead of the full Deps object
A lot of the functions which used a DepsMut as the input have been changed to take in &mut DepsMut, this was done due to many of the functions that are used in sequence each needing to have DepsMut provided, which is not a cloneable type

this is no longer required as authorization for invoking contract functions is done via RBAC now

Add new QueryMsg variant to return queued messages

version specific migrations, as well as updating the contract version in storage

cfkelly18 · 2024-05-22T15:29:55Z

initial review

x/ibc-rate-limit/contracts/rate-limiter/src/msg.rs

cfkelly18 · 2024-05-21T15:56:18Z

x/ibc-rate-limit/contracts/rate-limiter/src/state/flow.rs

+    /// The balance of a flow is how much absolute value for the denom has moved
+    /// through the channel before period_end. It returns a tuple of
+    /// (balance_in, balance_out) where balance_in in is how much has been
+    /// transferred into the flow, and balance_out is how much value transferred
+    /// out.
+    pub fn balance(&self) -> (Uint256, Uint256) {
+        (
+            self.inflow.saturating_sub(self.outflow),
+            self.outflow.saturating_sub(self.inflow),
+        )
+    }


[question]
im a little confused by this function, as it wouldnt show the absolute value if the saturating sub results in 0 right?

Yes I think that is correct, however this is from the original rate limits implementation so I'm not entirely sure what the intention is although I suspect it's meant as a way to prevent a panic from an overflow happening 🤔

x/ibc-rate-limit/contracts/rate-limiter/src/state/path.rs

x/ibc-rate-limit/contracts/rate-limiter/src/message_queue.rs

cfkelly18 · 2024-05-22T14:01:36Z

x/ibc-rate-limit/contracts/rate-limiter/src/rbac.rs

+/// Sets a timelock delay for `signer` of `hours`
+pub fn set_timelock_delay(
+    deps: &mut DepsMut,
+    signer: String,
+    hours: u64
+) -> Result<(), ContractError> {
+    Ok(TIMELOCK_DELAY.save(deps.storage, signer, &hours)?)
+}


[add validations]

I know that this is coming from a privileged address, and it also can just simply be reset if it was incorrectly set (but this would require a vote and cots time depending on the dao setup for that role)

Maybe enforce a config level max? and also make sure its not 0?

Signer should be validated to be a valid address

The validations take place within the execute function.

sorry I meant for msg.signer specifically. As this is just a string param passed into the msg. Maybe im missing something?

cfkelly18 · 2024-05-22T14:56:59Z

x/ibc-rate-limit/contracts/rate-limiter/src/execute.rs

+            Some(mut limits) => {
+                limits.iter_mut().for_each(|limit| {
+                    if limit.quota.name.eq(&quota.name) {
+                        // TODO: is this the current way of handling channel_value when editing the quota?
+
+                        // cache the current channel_value 
+                        let channel_value = limit.quota.channel_value;
+                        // update the quota
+                        limit.quota = From::from(&quota);
+                        // copy the channel_value
+                        limit.quota.channel_value = channel_value;
+                    }
+                });
+                Ok(limits)
+            }
+        }
+    })?;
+    Ok(())
+}


Responding to question -

is this the current way of handling channel_value when editing the quota

Seems good to me; the one thing we need to consider is a situation to where a quota is set to a value that is lower than the current channel value.

But in this scenerio I think it would behave as expected, it would just rate limit subsequent transfers

I think this would be a good question to ask the osmosis developers, but my assumption is that it would be good to keep the channel value for analytical purposes / ensuring the value correctly tracks the amount of value transferred in a given period.

yeah, we should keep the channel value. Otherwise we would be resetting the quota. The value will be updated on the next request anyway.

cfkelly18 · 2024-05-22T14:58:16Z

x/ibc-rate-limit/contracts/rate-limiter/src/execute.rs

@@ -108,6 +89,40 @@ pub fn try_reset_path_quota(
        .add_attribute("channel_id", channel_id))
 }

+pub fn edit_path_quota(


Maybe we could emit some more attributes from this fn? I think its fine either way though

I think more attributes is a good idea, just not sure what to add. Perhaps the parameters that were given when editing the quota?

x/ibc-rate-limit/contracts/rate-limiter/src/rbac.rs

x/ibc-rate-limit/contracts/rate-limiter/src/message_queue.rs

aesmonty · 2024-05-21T17:12:24Z

x/ibc-rate-limit/contracts/rate-limiter/src/contract.rs

-            env.block.time,
-        ),
+    // check to see if special permissions are required to invoke the message, and that the sender has the required permissions
+    crate::rbac::can_invoke_message(&deps, &info, &msg)?;


should we import the method above for clarity, like in other cases

Yea good idea (fixed in 3f3b87e)

aesmonty · 2024-05-21T17:16:35Z

x/ibc-rate-limit/contracts/rate-limiter/src/contract.rs

+    let contract_version = get_contract_version(deps.storage)?;
+
+    // check contract version, and apply version specific migrations
+    if contract_version.version.eq("0.1.0") {


is it good practice to hard-code here?

I'm not entirely certain to be honest, I wasn't able to find much documentation on best practices for contract migrations.

I think hard-coding is required as when a migration would need to be done, the contract version variable embedded into the contract would reference the version of the new contract as it's set during compilation.

I think this is fine. We only have one previous version to migrate from

aesmonty · 2024-05-21T17:31:42Z

x/ibc-rate-limit/contracts/rate-limiter/src/execute.rs

+            Some(mut limits) => {
+                limits.iter_mut().for_each(|limit| {
+                    if limit.quota.name.eq(&quota.name) {
+                        // TODO: is this the current way of handling channel_value when editing the quota?


is the TODO solved?

Sort of, this is my reply to Colin's review about this todo but ultimatley I think we need Osmosis' team to clarify.

x/ibc-rate-limit/contracts/rate-limiter/src/state/storage.rs

cfkelly18 · 2024-05-23T20:49:45Z

LGTM

cfkelly18 · 2024-05-28T13:29:55Z

x/ibc-rate-limit/contracts/rate-limiter/src/message_queue.rs

+pub fn process_message_queue(
+    deps: &mut DepsMut,
+    env: &Env,
+    count: Option<u64>,
+    message_ids: Option<Vec<String>>,
+) -> Result<Response, ContractError> {
+    let mut response = Response::new();
+    if let Some(message_ids) = message_ids {
+        let messages = pop_messages(deps.storage, &message_ids)?;
+        for mut message in messages {
+            // message_id isn't needed for message processing
+            // so avoid a clone by taking the message_id and replacing with empty string
+            let message_id = std::mem::take(&mut message.message_id);
+            if let Err(err) = try_process_message(deps, env, message) {
+                response = response.add_attribute(message_id, err.to_string());
+            } else {
+                response =  response.add_attribute(message_id, "ok");
+            }
+        }
+        return Ok(response);
+    }
+    let Some(count) = count else {
+        return Err(ContractError::InvalidParameters("both count and message_ids are None".to_string()))
+    };
    let queue_len = MESSAGE_QUEUE.len(deps.storage)? as usize;
-    
+
    for idx in 0..queue_len {
-        if idx + 1 > count {
+        if idx + 1 > count as usize {
            break;
        }
-        if let Some(message) = MESSAGE_QUEUE.pop_front(deps.storage)? {
-            // compute the minimum time at which the message is unlocked
-            let min_unlock = message
-            .submitted_at
-            .plus_seconds(message.timelock_delay * 60 * 60);
-            // check to see if the timelock delay has passed, which we need to first convert from hours into seconds
-            if env.block.time.ge(&min_unlock)
-            {
-                crate::contract::match_execute(deps, &env, message.message)?;
+        if let Some(mut message) = MESSAGE_QUEUE.pop_front(deps.storage)? {
+            let message_id = std::mem::take(&mut message.message_id);
+            if let Err(err) = try_process_message(deps, env, message) {
+                response = response.add_attribute(message_id, err.to_string());
            } else {
-                MESSAGE_QUEUE.push_back(deps.storage, &message)?;
+                response = response.add_attribute(message_id, "ok");
            }
        }
    }
-    Ok(())
+    Ok(response)
 }



two thoughts here:

we may want to store the errored messages in a separate item mapping rather than removing them and emitting an attribute. this way we can then allow a query to be made to query the failed message ids. This would help debugging failed messages, and also allow for a corrective message to be made. Overall this would just provide more visability
very very theoretical edge case: im not concerned about this in the real world

One downside to this FIFO pop queue approach, is that anybody can call the function and effectively push any specific message id to the back of the queue since the order is not preserved. For example if osmosis has a bot calling process messages count {10} every hour, an adversary could identify an important message id X and then call process messages with the id x when the queue time us T-1. This would push it to the back and delay the message processing if the queue is large or intentionally spammed. But this is also mostly mitigated with the ability to now process just a specific id. I dont see a need to fix this, but I wanted to raise the idea in case it conjured up some security considerations

nicolaslara

Did a pretty quick review, but this generally looks good to me. It would be great if you can make the PR to the osmosis repo so others from the team can review it.

We should also test this on testnet to see how it behaves and try to catch any potential errors.

bonedaddy · 2024-07-05T00:13:25Z

@nicolaslara Done osmosis-labs/osmosis#8481

bonedaddy added 7 commits May 16, 2024 20:46

state: add new state types, and storage variables

8e30850

(msg,contract): add new message types for ExecuteMsg and QueryMsg

ecf79f9

query: add functions to query allocated rbac permissions

78dc2ce

(msg,query): add query message to get proposal ids

4e5c894

rbac: add function to check rbac permission against message type

a90839b

state: change proposal queue to dequeue

3a71723

proposal_queue: wip functions for queueing, and processing proposals

28f8c5b

bonedaddy marked this pull request as draft May 17, 2024 06:47

bonedaddy added 22 commits May 16, 2024 23:59

contract: check rbac roles during execution, add set_timelock_delay

3f2eec8

rbac: add test for set_timelock_delay

b0f0cc2

chore: restructure state.rs

d87f936

(contract,execute) remove ibc + gov module sender address checks...

8c4369c

this is no longer required as authorization for invoking contract functions is done via RBAC now

contract: grant gov module all permissions during instantiation

ea1dc62

contract: grant gov module full permissions during migration

6d6f3c4

proposal_queue: fix proposal queue iteration counting

ef3023c

contract: check if ExecuteMsg must be queued before processing

47825dd

proposal: add tests for proposal queue check

8bcb103

chore: rename proposal to message

2272ead

query: take in storage trait argument instead of Deps....

b912e9c

Add new QueryMsg variant to return queued messages

contract: fix compilation

8e1ff8d

query: fix query tests

fcaab78

message_queue: add tests for proposal queue

141cfc5

message_queue: add complete testing of message queue functions

c50c722

rbac: use HashSet<T> for RBAC_PERMISSIONS storage variable

9d7b8bb

chore: finish renaming proposal -> message

faf8f5d

contract: add remaining ExecuteMsg handlers

35c6316

contract: update migrate function to perform ...

dbcb3af

version specific migrations, as well as updating the contract version in storage

rbac: test remaining functions

33f0ba7

chore: fix failing test cases

83cbd1d

tests: start adding integration tests for execute messages

ad8a8e9

tests: full execute message integration tests

afbcfbc

cfkelly18 reviewed May 22, 2024

View reviewed changes

x/ibc-rate-limit/contracts/rate-limiter/src/message_queue.rs Outdated Show resolved Hide resolved

chore: fix spelling errors

3822106

aesmonty reviewed May 23, 2024

View reviewed changes

bonedaddy added 3 commits May 23, 2024 11:40

chore: switch from HashSet to BTreeSet for determinism

7d33be3

chore: import function names in contract for better consistency

3f3b87e

rbac: enforce signer address validation

ab43f7c

bonedaddy added 4 commits May 27, 2024 16:20

contract: avoid queueing ExecuteMsg::ProcessMessages

122b04f

message_queue: wip count based or message id based queue processing

be68e19

message_queue: add remaining unit tests

8d78843

chore: format message_queue.rs

ed91d33

cfkelly18 reviewed May 28, 2024

View reviewed changes

bonedaddy marked this pull request as ready for review May 28, 2024 18:38

bonedaddy changed the title ~~[WIP] RBAC Permissions~~ RBAC Permissions May 28, 2024

nicolaslara approved these changes Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC Permissions #12

RBAC Permissions #12

bonedaddy commented May 17, 2024 •

edited

Loading

cfkelly18 commented May 22, 2024

cfkelly18 May 21, 2024

bonedaddy May 23, 2024

cfkelly18 May 22, 2024

bonedaddy May 23, 2024

cfkelly18 May 23, 2024

cfkelly18 May 22, 2024

bonedaddy May 23, 2024

nicolaslara Jul 4, 2024

cfkelly18 May 22, 2024

bonedaddy May 23, 2024

aesmonty May 21, 2024

bonedaddy May 23, 2024 •

edited

Loading

aesmonty May 21, 2024

bonedaddy May 23, 2024

nicolaslara Jul 4, 2024

aesmonty May 21, 2024

bonedaddy May 23, 2024

cfkelly18 commented May 23, 2024

cfkelly18 May 28, 2024

nicolaslara left a comment

bonedaddy commented Jul 5, 2024

RBAC Permissions #12

Are you sure you want to change the base?

RBAC Permissions #12

Conversation

bonedaddy commented May 17, 2024 • edited Loading

Changes

Questions

Notes

cfkelly18 commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonedaddy May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfkelly18 commented May 23, 2024

Choose a reason for hiding this comment

nicolaslara left a comment

Choose a reason for hiding this comment

bonedaddy commented Jul 5, 2024

bonedaddy commented May 17, 2024 •

edited

Loading

bonedaddy May 23, 2024 •

edited

Loading