sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

0xVinylDavyl - Contract does not implement proper access control mechanisms for the validateAndStore function, there are no checks to restrict access to authorized users. #25

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

0xVinylDavyl

medium

Contract does not implement proper access control mechanisms for the validateAndStore function, there are no checks to restrict access to authorized users.

Summary

The code does not implement proper access control mechanisms for the validateAndStore function. As a result, there are no checks to restrict access to authorized users.

Vulnerability Detail

The code does not implement proper access control mechanisms for the validateAndStore function. As a result, there are no checks to restrict access to authorized users,

Impact

The absence of access control mechanisms can have significant security implications:

Code Snippet

validateAndStore function is defined in the code without access control mechanisms:

function validateAndStore(
    RiskParameterStorage storage self,
    RiskParameter memory newValue,
    ProtocolParameter memory protocolParameter
) external {
    // Validation logic
    validate(newValue, protocolParameter);

    // Store new values in storage
    // ...
}

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/RiskParameter.sol#L173

there are no access control checks to restrict who can call the validateAndStore function.

Tool used

Manual Review

Recommendation

mitigate the risk of unauthorized modifications to the risk parameters, it is recommended to implement proper access control mechanisms. One common approach is to use the OpenZeppelin Ownable contract. Here's an example of how this can be done:

import "@openzeppelin/contracts/access/Ownable.sol";

contract YourContract is Ownable {
    // ... (other contract code)

    // Only the owner (contract deployer) can call this function
    function validateAndStore(
        RiskParameterStorage storage self,
        RiskParameter memory newValue,
        ProtocolParameter memory protocolParameter
    ) external onlyOwner {
        // Validation logic
        validate(newValue, protocolParameter);

        // Store new values in storage
        // ...
    }

    // ... (other contract functions)
}

By this recommended mitigation step, the Ownable contract from OpenZeppelin is imported, and the onlyOwner modifier is applied to the validateAndStore function. This ensures that only the contract owner (the deployer) can call the function, adding access control to prevent unauthorized modifications. Adjust the access control mechanism as needed to match your specific requirements.

sherlock-admin commented 11 months ago

4 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid because this is library function, meaning it's only implementation: smart contract has to delegatecall it to actually update the data using this function

n33k commented:

invalid, ai generated groundless report

darkart commented:

The same as 14

polarzero commented:

Invalid. As above, it is unclear what could happen in the worst case scenario.