Exchange Contracts - Pangolin

All functions that involve some kind of transfer of ownership require a single step, which is to set up the new privileged address. If this designated EOA account is not a valid account, it is very possible that the transfer of ownership will be made to an uncontrolled account, losing access to privileged functions.

PNG.sol

PNG.sol

function setMinter(address newMinter) external returns (bool) {
    require(msg.sender == admin, "Png::setMinter: unauthorized");
    emit MinterChanged(minter, newMinter);
    minter = newMinter;
    return true;
}

PNG.sol

function setAdmin(address newAdmin) external returns (bool) {
    require(msg.sender == admin, "Png::setAdmin: unauthorized");
    require(newAdmin != address(0), "Png::setAdmin: cannot make zero address the admin");
    emit AdminChanged(admin, newAdmin);
    admin = newAdmin;
    return true;
}

Airdrop.sol

Airdrop.sol

function setOwner(address owner_) external {
    require(owner_ != address(0), 'Airdrop::setOwner: invalid new owner');
    require(msg.sender == owner, 'Airdrop::setOwner: unauthorized');
    owner = owner_;
}

Airdrop.sol

    function setWhitelister(address addr) external {
        require(msg.sender == owner, 'Airdrop::setWhitelister: unauthorized');
        whitelister = addr;
    }

RISK ACCEPTED: Pangolin team accepts this risk:

• Airdrop events will last for a short time, and it is expected that ownership will only be transferred once from the deployer to the multisig.
• PNG Ownership will only be transferred from the deployer to the Timelock contract (contracts/governance/Timelock.sol). This Timelock contract has the suggested two-step ownership transfer pattern, and any additional change of ownership would likely go through Timelock rather than PNG.

In the contract TreasuryVester the distribute() function is used to distribute the tokens to recipients based on their allocation:

TreasuryVester.sol

TreasuryVester.sol

function distribute() public {
    require(vestingEnabled, "TreasuryVester::distribute: vesting is not enabled");
    require(
        block.timestamp >= lastUpdate + VESTING_CLIFF,
        "TreasuryVester::distribute: it is too early to distribute"
    );
    lastUpdate = block.timestamp;

    // defines a vesting schedule that lasts for 30 months
    if (step % STEPS_TO_SLASH == 0) {
        uint slash = step / STEPS_TO_SLASH;
        if (slash < 5) {
            _vestingPercentage = _initialVestingPercentages[slash];
        } else if (slash < 12) {
            _vestingPercentage -= 20;
        } else if (slash < 20) {
            _vestingPercentage -= 15;
        } else if (slash < 30) {
            _vestingPercentage -= 10;
        } else {
           revert("TreasuryVester::distribute: vesting is over");
        }
        _vestingAmount = getVestingAmount();
    }
    step++;

    // distributes _vestingAmount of tokens to recipients based on their allocation
    for (uint i; i < _recipientsLength; i++) {
        Recipient memory recipient = _recipients[i];
        uint amount = recipient.allocation * _vestingAmount / DENOMINATOR;
        if (!recipient.isMiniChef) {
            // simply mints or transfer tokens to regular recipients
            vestedToken.mint(recipient.account, amount);
        } else {
            // calls fund rewards of minichef after minting tokens to self
            vestedToken.mint(address(this), amount);
            vestedToken.approve(recipient.account, amount);
            IMiniChefV2(recipient.account).fundRewards(amount, VESTING_CLIFF);
        }
    }
    emit TokensVested(_vestingAmount);
}

\color{black} \color{white}

As the length of recipients is not limited, in case that there are too many recipients, the block gas limit could be reached, causing miners to not respond to all distribute() calls, thus blocking the main purpose of the smart contract.

SOLVED: Pangolin team solved the issue in the commit id 1e2f374c6728b998a22045a673be0ba14156b9c1.

In the TreasuryVester and Airdrop contracts, the constructors are missing address validation. On the other hand, in some functions it is also critical to perform this validation. For example, in the TreasuryVester.setRecipients() function, if a 0 address recipient was set the distribute() call would always fail, since most tokens cannot be minted at the 0 address.

Each address should be validated and checked to be non-zero.

TreasuryVester.sol

TreasuryVester.sol

constructor(
    address newVestedToken,
    uint newStartingBalance,
    Recipient[] memory newRecipients,
    address newGuardian
) {
    require(newStartingBalance > 0, "TreasuryVester::Constructor: invalid starting balance");
    require(newGuardian != address(0), "TreasuryVester::Constructor: invalid guardian address");
    guardian = newGuardian;
    vestedToken = IPng(newVestedToken);
    startingBalance = newStartingBalance;
    setRecipients(newRecipients);
}

TreasuryVester.sol

function setRecipients(Recipient[] memory newRecipients) public onlyOwner {
    _recipientsLength = newRecipients.length;
    require(
        _recipientsLength != 0,
        "TreasuryVester::setRecipients: invalid recipient number"
    );
    uint allocations;
    for (uint i; i < _recipientsLength; ++i) {
        Recipient memory recipient = newRecipients[i];
        require(
            recipient.account != address(0),
            "TreasuryVester::setRecipients: invalid recipient address"
        );
        require(
            recipient.allocation != 0,
            "TreasuryVester::setRecipients: invalid recipient allocation"
        );
        _recipients[i] = recipient;
        allocations += recipient.allocation;
    }
    require(
        allocations == DENOMINATOR,
        "TreasuryVester::setRecipients: invalid total allocation"
    );
    emit RecipientsChanged(newRecipients);
}

Airdrop.sol

Airdrop.sol

constructor(
    uint supply_,
    address png_,
    address owner_,
    address remainderDestination_
) {
    airdropSupply = supply_;
    png = png_;
    owner = owner_;
    remainderDestination = remainderDestination_;
}

Airdrop.sol

function setRemainderDestination(address remainderDestination_) external {
    require(
        msg.sender == owner,
        'Airdrop::setRemainderDestination: unauthorized'
    );
    remainderDestination = remainderDestination_;
}

PNG.sol

PNG.sol

function setMinter(address newMinter) external returns (bool) {
    require(msg.sender == admin, "Png::setMinter: unauthorized");
    emit MinterChanged(minter, newMinter);
    minter = newMinter;
    return true;
}

SOLVED: Pangolin team solved the issue in the commit id 8810acc38f27bf9de25a492ffe41d3c54c657c5f.

In some for loops, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

TreasuryVester.sol

• Line 149: for (uint i; i < _recipientsLength; i++) {

Airdrop.sol

• Line 178: for (uint i = 0; i < addrs.length; i++) {

For example, based in the following test contract:

Test.sol

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

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

SOLVED: Pangolin team solved the issue in the commit id 4aa439b03eac34bebf40c6b72e9afeb2d0fa2333.

Since i is an uint, it is already initialized to 0. uint i = 0 reassigns the 0 to i which wastes gas. The same applies to the lower state variable shown below.

Code Location

Airdrop.sol

• Line 178: for (uint i = 0; i < addrs.length; i++) {

PNG.sol

• Line 373: uint32 lower = 0;

SOLVED: Pangolin team solved the issue in the commit id 87781621d7294afeafd7a33916b4016dc1f3ed34.

In the TreasuryVester and PNG contracts, there are some functions marked as public that are never called directly within the contract itself or in any of their descendants:

TreasuryVester.sol

• distribute() (TreasuryVester.sol#122-163)

PNG.sol

• delegate() (PNG.sol#314-316)
• delegateBySig() (PNG.sol#327-336)
• getPriorVotes() (PNG.sol#355-387)

SOLVED: Pangolin team solved the issue in the commit id f1fff4b75db0fe70450c55234418bd445834029d.