Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRSRY:getLoan() is permissioned, but no policy has permission to call it #389

Open
code423n4 opened this issue Sep 1, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/TRSRY.sol#L92

Vulnerability details

Impact

The getLoan() function in the TRSRY.sol contract is a way for users to withdraw approved loans from the protocol, which can be repaid with repayLoan().

No policies have permission to call this function, and it appears it was intended to be publicly callable, but it has the permissioned modifier, so it is functionally useless.

Without the getLoan() function, repayLoan() is functionally useless as well.

Proof of Concept

Checking the permissions set in each of the policies, there is no policy that has permission to call getLoan().

The permissioned modifier requires that the function is only called by policies that have been pre-approved.

/// @notice Modifier to restrict which policies have access to module functions.
modifier permissioned() {
    if (!kernel.modulePermissions(KEYCODE(), Policy(msg.sender), msg.sig))
        revert Module_PolicyNotPermitted(msg.sender);
    _;
}

As a result, this function cannot be called under any circumstances.

Tools Used

VS Code

Recommended Mitigation Steps

Remove the permissioned modifier from the getLoan() function.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 1, 2022
code423n4 added a commit that referenced this issue Sep 1, 2022
@0xLienid 0xLienid added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Sep 6, 2022
@0xLienid
Copy link
Collaborator

0xLienid commented Sep 6, 2022

The system is designed around the ability to add and remove policies in the future. The fact there is not a policy using this feature on launch does not preclude its future use and need for permissioning.

@0xean
Copy link
Collaborator

0xean commented Sep 16, 2022

Downgrading to QA, sponsors may want to consider documenting that its intended for future use.

@0xean 0xean added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants