search

code-423n4 / 2022-11-redactedcartel-findings

3 stars 2 forks source link

Verification on contract/address is too loose, which may lead to serious implication #338

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L182-L191 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexRewards.sol#L94 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PxERC20.sol#L30 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L75 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L82 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PxGmxReward.sol#L41

Vulnerability details

Instances: (44):

Link to code

File: src/PirexGmx.sol

182:         if (_pxGmx == address(0)) revert ZeroAddress();

183:         if (_pxGlp == address(0)) revert ZeroAddress();

184:         if (_pirexFees == address(0)) revert ZeroAddress();

185:         if (_pirexRewards == address(0)) revert ZeroAddress();

186:         if (_delegateRegistry == address(0)) revert ZeroAddress();

187:         if (_gmxBaseReward == address(0)) revert ZeroAddress();

188:         if (_gmx == address(0)) revert ZeroAddress();

189:         if (_esGmx == address(0)) revert ZeroAddress();

190:         if (_gmxRewardRouterV2 == address(0)) revert ZeroAddress();

191:         if (_stakedGlp == address(0)) revert ZeroAddress();

317:         if (contractAddress == address(0)) revert ZeroAddress();

497:         if (token == address(0)) {

599:         if (token == address(0)) revert ZeroAddress();

829:         if (token == address(0)) revert ZeroAddress();

926:         if (newContract == address(0)) revert ZeroAddress();

Link to code

File: src/PirexRewards.sol

94:         if (_producer == address(0)) revert ZeroAddress();

112:         if (address(producerToken) == address(0)) revert ZeroAddress();

113:         if (address(rewardToken) == address(0)) revert ZeroAddress();

136:         if (address(producerToken) == address(0)) revert ZeroAddress();

137:         if (address(rewardToken) == address(0)) revert ZeroAddress();

155:         if (address(producerToken) == address(0)) revert ZeroAddress();

156:         if (address(rewardToken) == address(0)) revert ZeroAddress();

183:         if (address(producerToken) == address(0)) revert ZeroAddress();

271:         if (address(producerToken) == address(0)) revert ZeroAddress();

282:         if (address(producerToken) == address(0)) revert ZeroAddress();

374:         if (address(producerToken) == address(0)) revert ZeroAddress();

439:         if (address(producerToken) == address(0)) revert ZeroAddress();

440:         if (address(rewardToken) == address(0)) revert ZeroAddress();

467:         if (address(producerToken) == address(0)) revert ZeroAddress();

468:         if (address(rewardToken) == address(0)) revert ZeroAddress();

Link to code

File: src/PxERC20.sol

30:         if (_pirexRewards == address(0)) revert ZeroAddress();

Link to code

File: src/vaults/AutoPxGlp.sol

75:         if (_gmxBaseReward == address(0)) revert ZeroAddress();

76:         if (_asset == address(0)) revert ZeroAddress();

79:         if (_platform == address(0)) revert ZeroAddress();

80:         if (_rewardsModule == address(0)) revert ZeroAddress();

131:         if (_platform == address(0)) revert ZeroAddress();

374:         if (token == address(0)) revert ZeroAddress();

Link to code

File: src/vaults/AutoPxGmx.sol

82:         if (_gmxBaseReward == address(0)) revert ZeroAddress();

83:         if (_gmx == address(0)) revert ZeroAddress();

84:         if (_asset == address(0)) revert ZeroAddress();

87:         if (_platform == address(0)) revert ZeroAddress();

88:         if (_rewardsModule == address(0)) revert ZeroAddress();

153:         if (_platform == address(0)) revert ZeroAddress();

Link to code

File: src/vaults/PxGmxReward.sol

41:         if (_pxGmx == address(0)) revert ZeroAddress();

Impact

Priex encompasses many smart contracts. Consequently, there are many address/contract assignments. However, addresses and contracts are assigned to state variables under very loose verification, i.e. a non-zero address will pass the validation. This may lead to serious implications or even chaos if an inappropriate contract address is assigned in use since there is no way to predict the consequences. So, it is necessary to take every measure to prevent wrong contracts in use.

Tools Used

Manual audit.

Recommended Mitigation Steps

ERC165 is a standard to detect and publish what interfaces a smart contract implements. This can be used to verify whether a contract address supports specific interfaces, which can dramatically reduce the risk of using a wrong contract. Openzeppelin has implemented the standard and we can use it for our purpose. The below is an example of how to use ERC165 tools from Openzeppelin to implement contract interface verification. Contracts of Priex should implement this mechanism where possible so as to reduce the risk of using wrong contracts.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import {ERC165Storage} from "@openzeppelin/contracts/utils/introspection/ERC165Storage.sol";

interface IArbitraryTokenStorage {
    function withdrawToOwner(IERC20 token) external;
}

contract NewPxToken is ERC165Storage, IArbitraryTokenStorage, ERC20 {
    address public owner;

    error NoEnoughBalance();
    error TransferFailed();

    constructor() ERC20("New Px Token", "NPT") {
        owner = msg.sender;
        // to register function interfaces in constructor or initialize function
        _registerInterface(type(IArbitraryTokenStorage).interfaceId);
        _registerInterface(type(IERC20).interfaceId);
    }

    function withdrawToOwner(IERC20 token) external {
        uint256 balance = token.balanceOf(address(this));

        if(balance == 0) revert NoEnoughBalance();
        if(!token.transfer(owner, balance)) revert TransferFailed();
    }
}

contract SetNewPxToken {
    using ERC165Checker for address;

    NewPxToken public newPxToken;

    error NotSupportArbitraryTokenStorage();
    error NotSupportERC20();

    function setContractInstance(address _addr) public {
        // verify whether the input address `_addr` supports required interfaces
        if(!_addr.supportsInterface(type(IArbitraryTokenStorage).interfaceId)) revert NotSupportArbitraryTokenStorage();
        if(!_addr.supportsInterface(type(IERC20).interfaceId)) revert NotSupportERC20();
        newPxToken = NewPxToken(_addr);
    }
}
Picodes commented 1 year ago

This falls within QA as it is a suggestion to add deployments safety checks.

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/330