obront - Using one controller for two addresses could risk signature collisions

[github-actions]

obront

medium

Using one controller for two addresses could risk signature collisions

Summary

The DNGMXVaultController is intended to be used as a controller for two separate contracts when interacting with Rage Trade. While individual contracts check for signature collisions, there is no protection in this Controller. If the contracts end up with functions with colliding signatures, this may enable users to call illegal functions and get around the protocol safeguards.

Vulnerability Detail

While most of Sentiment's Controllers correspond to one contract, the DNGMXVaultController is one controller that will support two separate contracts: DepositPeriphery and WithdrawPeriphery.

While signature collisions are unlikely, they do happen. For this reason, the Solidity compiler stops code from compiling in the case of a collision. This ensures that there will not be issues in any EVM bytecode. Because Sentiment controllers approve function calls based on signatures, they can be sure that, for any given contract, they will only be approving the function they are intending.

However, once multiple contracts are managed by one controller, there are no compiler checks across these contracts for colliding signatures. The result is that there is the potential for a function signature on one contract that is approved, due to a matching signature on the other contract.

Impact

There is the potential for users to get access to non-permitted functions if there is a signature collision.

This is a security risk in this specific case, but is also a more global suggestion for Sentiment. While two contracts on one Controller one time is unlikely to cause a problem, the practice of loading multiple contracts onto on Controller should be avoided, as the risks will increase as this is performed.

Code Snippet

These three functions do not exist on the same contract:

https://github.com/sherlock-audit/2023-01-sentiment/blob/main/controller-52/src/rage/DNGMXVaultController.sol#L15-L22

Tool used

Manual Review

Recommendation

Split DNGMXVaultController into two files, one for each of the two contracts that will be interacted with.

Going forward, continue to uphold the practice of always having one Controller per contract, unless the two contracts are identical and non-upgradeable.

[bahurum]

Escalate for 50 USDC.
This issue should be low/informational.
I have flagged the issue but as Informational (#25), since the two target contracts are non-upgradeable and new functions can never be added and signature collision is impossible.
Here are the contracts on chain:
depositPeriphery
withdrawPeriphery
Note that they do not share any of the 3 function signatures of DNGMXVaultController and they never will since they are non-upradeable.

[sherlock-admin]

Escalate for 50 USDC.
This issue should be low/informational.
I have flagged the issue but as Informational (#25), since the two target contracts are non-upgradeable and new functions can never be added and signature collision is impossible.
Here are the contracts on chain:
depositPeriphery
withdrawPeriphery
Note that they do not share any of the 3 function signatures of DNGMXVaultController and they never will since they are non-upradeable.

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[zobront]

Baharum's issue did not identify the signature collision as a risk for combining contracts into one Controller, and simply said that it would be confusing, which is why it was identified as Informational.

The issue here points out that the current Controller violates an explicit practice that Sentiment must be enforcing on all contracts to keep their protocol safe. Just because the collision doesn't happen in this specific case, it doesn't change the fact that Sentiment is violating a security principle that they should be upholding consistently.

To drive the point home, I do not believe Sentiment would get their contracts re-audited if they added one function to an individual Controller. So this type of issue needs to be caught NOW so that they don't set themselves up to be in a situation where adding one innocent, safe function later ends up causing a catastrophic problem.

[zobront]

Fix confirmed.

[hrishibhat]

Escalation accepted.

After further internal discussion, considering this issue as informational as there are no collision risks with current implementation and any changes to the code must undergo an audit process.

[sherlock-admin]

Escalation accepted.

After further internal discussion, considering this issue as informational as there are no collision risks with current implementation and any changes to the code must undergo an audit process.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.