"adminProxy" Contract Misuse Vulnerability.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L545-L550 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L561 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L579 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L605 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L618

Vulnerability details

Impact

Detailed description of the impact of this finding.

(bool success, bytes memory returnData) = adminProxy.execute(
      address(escrow),
      abi.encodeWithSelector(IMultiRewardEscrow.setFees.selector, tokens, fees)
    );
    if (!success) revert UnderlyingError(returnData);
  }

and it is repeated in the following functions as well.

addTemplateCategories
toggleTemplateEndorsements
pauseAdapters
pauseVaults

When the adminProxy contract is compromised, it could lead to the loss of funds or execution of unintended functions in the target contract, potentially resulting in financial loss or damage to the system.

Proof of Concept

1. The attacker deploys a malicious contract with a similar interface as the intended adminProxy contract.

2. The attacker then tricks the owner of the vulnerable contract into using their malicious contract instead of the intended adminProxy.

3. The attacker then implements the execute function in a way that allows them to steal funds or manipulate data.

4. The owner of the vulnerable contract, thinking they are using the intended adminProxy, executes a contract with the malicious execute function, causing a loss of funds or data manipulation.

Code for the malicious contract.

pragma solidity ^0.8.0;

contract MaliciousAdminProxy {
    function execute(address _to, bytes memory _data) public {
        // Steal funds or manipulate data as desired by the attacker.
        // Example code to steal all funds:
        _to.call.value(address(this).balance)(_data);
    }
}

Tools Used

Manual audit, Remix IDE, VSCODE.

Recommended Mitigation Steps

1. Verify the implementation of the adminProxy contract before using it in the main contract. Ensure that the adminProxy contract only has the desired functionality and does not contain any malicious code.

2. Use a trusted adminProxy contract, such as one developed and audited by a reputable third-party, to reduce the risk of compromise.

3. Implement access control in the main contract to limit the functionality of the adminProxy contract. For example, limit the ability of the adminProxy contract to only execute certain specific contracts with a specific selector and data.

4. Consider implementing a fail-safe mechanism in the main contract that can be triggered in the event of a security breach or malicious activity by the adminProxy contract. This can be achieved by using a time lock mechanism or a mechanism to allow the owner to recover control of the contract in the event of a security breach.

Code Implementation.

pragma solidity ^0.8.0;

contract MainContract {
    address public owner;
    address public adminProxy;
    bytes4 public selector;

    constructor(address _adminProxy, bytes4 _selector) public {
        owner = msg.sender;
        adminProxy = _adminProxy;
        selector = _selector;
    }

    function execute(address target, bytes memory data) public {
        // Check that the adminProxy contract is trusted
        require(adminProxy == address(0x1234567890123456789012345678901234567890), "Invalid adminProxy contract");
        // Check that the selector passed is valid
        require(selector == 0x12345678, "Invalid selector");
        // Check that the target contract is trusted
        require(target == address(0x09876543210987654321098765432109876543210), "Invalid target contract");

        (bool success, ) = address(adminProxy).delegatecall(data);
        require(success, "Execution failed");
    }

    function updateAdminProxy(address _adminProxy) public {
        require(msg.sender == owner, "Only the owner can update the adminProxy contract");
        adminProxy = _adminProxy;
    }

    function updateSelector(bytes4 _selector) public {
        require(msg.sender == owner, "Only the owner can update the selector");
        selector = _selector;
    }
}

In this code, the MainContract checks the validity of the adminProxy contract and the selector before executing the desired contract. The updateAdminProxy and updateSelector functions can only be called by the owner and allow for updating the adminProxy contract and selector in the event of a security breach or change in requirements.

[c4-sponsor]

RedVeil marked the issue as sponsor disputed

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid