howlbot-integration[bot] commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/govshuttle/keeper/msg_server.go#L30 https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/govshuttle/keeper/msg_server.go#L48

Vulnerability details

Impact

GovShuttle module misses validation for duplicate propId which might result in a proposal being rewritten and funds stolen.

Summary

An attacker waits before a valid proposal is approved. After approval they can rewrite it using the govshuttle with any target contract, data and value. Since govshuttle is an admin of other contracts this will allow an attacker to steal funds and become an admin of any contract controlled by the module.

Recommended Mitigation Steps

Make sure to check fail if a proposal already exists before calling AppendLendingMarketProposal.

Affected lines: https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/govshuttle/keeper/msg_server.go#L30

https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/govshuttle/keeper/msg_server.go#L48

Assessed type

Invalid Validation

poorphd commented 5 months ago

Labels
- sponsor acknowledged
Reasoning
- It's accurate that the current Solidity and GovShuttle code don't perform duplicate checks on proposals, which could potentially be rewritten.
- However, in the AddProposal section of the port.sol contract used by GovShuttle to register proposals, there's a requirement statement: (see AddProposal)
```
require(msg.sender == govshuttleModAcct); // only GovShuttle account can add proposals to store
```
- This means only proposals approved by the governance process can be added. In the scenario you outlined, an attacker would need to wait until a valid proposal is approved, then immediately rewrite it. The duplicate proposal would also need consecutive governance approval.
- A proposal must receive a quorum of 'yes' votes to pass, so the chance of a malicious duplicate proposal passing consecutively is extremely low.
- Considering such a malicious governance scenario as a vulnerability isn't feasible. Many Cosmos base modules have various parameters. Malicious changes to parameters like MintDenom, BondDenom, MaxValidators, or inflation could halt the chain, enable a takeover, or cause financial loss. But, these aren't considered vulnerabilities. We can't acknowledge this as a vulnerability resulting from governance actions.
Severity
- High→Low
- Even if a malicious proposal passes, during the querying and queuing process in (GovernorBravoDelegate.sol), duplicate checks are carried out. If the original proposal is already queued, it prevents another proposal with the same ID from being queued:
```
// Make sure you are not overriding an existing proposal
require(proposals[unigovProposal.id].id == 0, "GovernorBravo::queue: Proposal has already been queued");
```
- Seeing that there is technically no duplicate check, we've labeled this issue as acknowledged and suggest a severity of Low.

3docSec commented 5 months ago

Hi @poorphd could you please elaborate more on this argument:

This means only proposals approved by the governance process can be added

The code you quoted immediately before this only makes sure that it's govshuttle who calls the contract, but I miss the validation that govshuttle can only be called by the governance module.

Can't anyone send a MsgLendingMarketProposal to govshuttle? Other modules validate proposals such as MsgUpdateParams by checking the signer (Authority), but govshuttle doesn't seem like doing so, therefore it seems to me that the governance can be bypassed, making the attack described in the report realistic

poorphd commented 5 months ago

Hi @3docSec, thanks for your review.

Yes, that's right. After freezing the audit target code, our team internally identified that the authority validation for the govshuttle’s proposal msgs was missing, separate from the report. This issue was intended to be patched internally. We understood the report to have focused on the misses validation for duplicate propId, because the report didn’t mentioning the authority checks as a mitigation and Impact, only addressing the propId existence check.

However, given that the authority validation issue has been clarified through your argument, even upon re-evaluation, our team has found that the audit target code has issues with proto, codec, and marshaling not functioning correctly for several Msgs including govshuttle proposal msgs. Therefore, even if the authority validation is missing, it appears that user can’t send a proposal msg, making the attack scenario you mentioned impossible.

unsigned_tx.json
{
    "body": {
        "messages": [
            {
                "@type": "/canto.govshuttle.v1.MsgLendingMarketProposal",
                "Authority": "canto10d07y265gmmuvt4z0w9aw880jnsr700jg5j4zm",
                "Title": "test proposal",
                "Description": "test",
                "Metadata": "test"
            }
        ],
        "memo": "",
        "timeout_height": "0",
        "extension_options": [],
        "non_critical_extension_options": []
    },
    "auth_info": {
        "signer_infos": [],
        "fee": {
            "amount": [],
            "gas_limit": "200000",
            "payer": "",
            "granter": ""
        },
        "tip": null
    },
    "signatures": []
}

$ cantod tx sign unsigned_tx.json --from canto1zjt3k88vft0vualyzv3qty6avf895qqyv59e3w --chain-id canto_9000-1 --output-document signed_tx.json

Error: can't unmarshal Any nested proto *types.MsgLendingMarketProposal: unknown field "Authority" in types.MsgLendingMarketProposal: tx parse error

If you could provide a PoC code or script demonstrating that the attack scenario you mentioned is possible within this context/audit target code, we would be able to reconsider our assessment.

3docSec commented 5 months ago

I see what you mean, at the very least there is #8 that prevents sending direct messages to the Govshuttle module to bypass the governance.

I agree with you, an attack would need to pass a governance proposal, which makes it a low risk scenario.

c4-judge commented 5 months ago

3docSec changed the severity to QA (Quality Assurance)