Plural Energy Offering - Plural Energy

Any user could call the extendTime of the offering created by the admin. There was a lack of onlyOwner modifier in the extendTime public function. Ensure that administration mechanism as extend the offering timestamp when purchasing should no controlled by malicious actors.

The following test showed how the attacker could extend the time on a particular offer created by the admin.

Exploit:

function purchase(bytes16 buyerStakeholderId) public returns (uint) {
        vm.assume(buyerStakeholderId != bytes16(0));
        vm.assume(buyerStakeholderId != issuerStakeholderId);
        vm.assume(buyerStakeholderId != offeringStakeholderId);


        // Setup the initial issuance of the offering
        vm.startPrank(_admin);
        _offering.setupInitialIssuance(_issuedSupply, _issuer, _issuerAmount);
        vm.stopPrank();

        // buyer purchases

        uint64 tokensToPurchase1 = 25;
        uint64 tokensToPurchase2 = 25;
        uint64 tokensToPurchase3 = 50;
        uint64 tokensToPurchase = 100;
        uint tokensToPurchaseWithDecimals = tokensToPurchase *
            (10 ** IERC20(_token).decimals());
        uint pricePerToken = 10e6; // $1
        uint paymentAmount = tokensToPurchase * pricePerToken;
        string memory orderId = "123";

        // Issuer approves Vendor contract to transfer tokens
        setupPurchaser(_buyer, buyerStakeholderId);

        // Buyer approves Vendor contract to purchase tokens with usdc
        // Vendor contract == _offering.
        vm.startPrank(_buyer);
        _usdc.approve(address(_offering), paymentAmount);
        assertEq(IERC20(_token).balanceOf(address(_offering)), 0);

        _offering.purchase(_buyer, tokensToPurchase1, orderId);
        
        _offering.purchase(_buyer, tokensToPurchase2, orderId);
        _offering.extendTime(1672531311); //changing the time as attacker.
        _offering.isOpen();
        console2.log(_offering.closingTime()); //check that the attacker extend the time.
        _offering.purchase(_buyer, tokensToPurchase3, orderId);
        vm.stopPrank();

        return tokensToPurchaseWithDecimals;
    }

Ensure the use of onlyOwner modifier on that function.

Remediation Plan

SOLVED: The Plural Energy team solved the issue by adding onlyOwner modifier.

function extendTime(uint40 newClosingTime) public override onlyOwner {
        require(isOpen(), "Offering already closed");
        // solhint-disable-next-line max-line-length
        require(
            newClosingTime > closingTime,
            "TimedCrowdsale: new closing time is before current closing time"
        );

        closingTime = newClosingTime;
    }

The orderId is not properly validated on the Token purchase if it already exists in previous purchases by the buyers. Basically, the orderId is not validated on the purchase function. This issue arises in the emit event that the orderId is not checked if already purchases exists or not before. Moreover, the buyer user could purchase using the same orderId parameter twice. If the emit event is used in an off chain application could be a critical issue that the same orderId could be treated as the same from different buyers.

After confirmation with the client, the issue is valid because currently the event would be used directly. Basically, the order ID maps to the id of an order record in their database. Through the purchase event containing the order id, they can link the on chain transaction to the order in their systems and confirm that the asset tokens were transferred from treasury to purchaser and payment tokens from purchaser to the payment router. This is important from a regulatory standpoint as they can use this to prove that the transfer of assets happened (settlement, in the process of execution, clearing, and settlement of a security transaction).

The following test showed how a buyer could use the same orderId in each purchase. That event would be use on the offchain component and several same orderId existing could allow errors on the database queries.

Exploit:

function purchase(bytes16 buyerStakeholderId) public returns (uint) {
        vm.assume(buyerStakeholderId != bytes16(0));
        vm.assume(buyerStakeholderId != issuerStakeholderId);
        vm.assume(buyerStakeholderId != offeringStakeholderId);


        // Setup the initial issuance of the offering
        vm.startPrank(_admin);
        _offering.setupInitialIssuance(_issuedSupply, _issuer, _issuerAmount);
        vm.stopPrank();

        // buyer purchases

        uint64 tokensToPurchase1 = 25;
        uint64 tokensToPurchase2 = 25;
        uint64 tokensToPurchase3 = 50;
        uint64 tokensToPurchase = 100;
        uint tokensToPurchaseWithDecimals = tokensToPurchase *
            (10 ** IERC20(_token).decimals());
        uint pricePerToken = 10e6; // $1
        uint paymentAmount = tokensToPurchase * pricePerToken;
        string memory orderId = "123";

        // Issuer approves Vendor contract to transfer tokens
        setupPurchaser(_buyer, buyerStakeholderId);

        // Buyer approves Vendor contract to purchase tokens with usdc
        // Vendor contract == _offering.
        vm.startPrank(_buyer);
        _usdc.approve(address(_offering), paymentAmount);
        assertEq(IERC20(_token).balanceOf(address(_offering)), 0);

        _offering.purchase(_buyer, tokensToPurchase1, orderId); 
        _offering.purchase(_buyer, tokensToPurchase2, orderId); //using same order id.
        _offering.purchase(_buyer, tokensToPurchase3, orderId); //using same order id.
        vm.stopPrank();

        return tokensToPurchaseWithDecimals;
    }

Implementing proper validation on the orderId.

Remediation Plan

SOLVED : The Plural Energy team solved the issue by adding proper validation on validatePurchase function.

function validatePurchase(
        address purchaser,
        uint paymentAmount,
        uint64 amountRequested,
        string calldata orderId
    ) public view override {
        validateTransfer(purchaser, amountRequested);
        require(
            paymentRouter != address(0),
            "Payment router is the zero address"
        );
        require(paymentAmount != 0, "Payment amount is 0");
        require(
            IERC20(paymentToken).allowance(purchaser, address(this)) >=
                paymentAmount,
            "Payment allowance too low"
        );
        if (orders[orderId].purchaser != purchaser) {
            revert OrderWrongBuyer(purchaser, orders[orderId].purchaser);
        }
        if (orders[orderId].assetTokenAmount != amountRequested) {
            revert OrderWrongAmount(
                amountRequested,
                orders[orderId].assetTokenAmount
            );
        }
        if (orders[orderId].purchased) {
            revert OrderAlreadyPurchased(orderId);
        }
        // TODO: check any cap table restrictions (e.g. limits for certain stakeholders)
        // offering.validatePurchase();
    }

Some ERC20 token implementations have a fee that is charged on each token transfer. This means that the transferred amount isn't exactly what the receiver will get. The main issue then lies in that the protocol will pull funds from arbitrary ERC20 token implementation using transferFrom, assuming that the receiving amount matches the amount sent as the parameter to the transfer call, which is not always the case.

/// @inheritdoc IOffering
    function purchase(
        address purchaser,
        uint64 assetTokenAmount,
        string calldata orderId
    ) external override nonReentrant whenNotPaused {
        IERC20 _paymentToken = IERC20(paymentToken);
        uint paymentAmount = sharePrice * assetTokenAmount;

        validatePurchase(purchaser, paymentAmount, assetTokenAmount);

        // Take payment from purchaser and transfer to the payment router.
        require(
            _paymentToken.transferFrom(purchaser, paymentRouter, paymentAmount),
            "Payment failed"
        );

        _transferFromVendor(purchaser, assetTokenAmount, orderId);
    }

It is recommended to improve support for USDT type of ERC20. When pulling funds from the user using transferFrom the usual approach is to compare balances pre/post-transfer, like so:

// Take payment from purchaser and transfer to the payment router.
        uint256 balanceBefore = IERC20(paymentToken).balanceOf(address(this));
        require(
            _paymentToken.transferFrom(purchaser, paymentRouter, paymentAmount),
            "Payment failed"
        );
        uint256 transferred = IERC20(paymentToken).balanceOf(address(this)) - balanceBefore;

Remediation Plan

SOLVED : The Plural Energy team solved the issue by adding safeTransferFrom function.

function purchase(
        address purchaser,
        uint64 assetTokenAmount,
        string calldata orderId
    ) external override nonReentrant whenNotPaused {
        IERC20 _paymentToken = IERC20(paymentToken);
        uint paymentAmount = sharePrice * assetTokenAmount;

        validatePurchase(purchaser, paymentAmount, assetTokenAmount, orderId);

        // Take payment from purchaser and transfer to the payment router
        IERC20(_paymentToken).safeTransferFrom(
            purchaser,
            paymentRouter,
            paymentAmount
        );

        _transferFromVendor(purchaser, assetTokenAmount, orderId);
    }

The contract uses block.timestamp. The global variable block.timestamp does not necessarily hold the current time, and may not be accurate. Miners can influence the value of block.timestamp to perform Maximal Extractable Value (MEV) attacks. There is no guarantee that the value is correct, only that it is higher than the previous block’s timestamp.

Use block.number instead of block.timestamp to reduce the risk of MEV attacks. If possible, use an oracle.

Remediation Plan

ACKNOWLEDGED : The Plural Energy team acknowledged the issue.

During the assessment it has been observed than the offeringAddress is not properly validated. If the offering is deleted and after approved, the EVM revert is triggered due to the safeApprove function is using as second parameter the address zero due to empty index mapping after deletion getter (getOffering function).

/// @inheritdoc IAssetTokenVendor
    function approveOffering(
        address assetToken,
        uint32 offeringNum
    ) public override onlyOwner {
        address offeringAddress = getOffering(assetToken, offeringNum);

        // Ensure compliance rules are updated for the new offering
        IAssetToken _assetToken = IAssetToken(assetToken);
        ICompliance compliance = ICompliance(_assetToken.compliance());
        address[] memory tokenAddresses = new address[](1);
        tokenAddresses[0] = assetToken;
        string memory allowedRole = Roles.OFFERING;
        string[] memory roles = new string[](1);
        roles[0] = allowedRole;

        compliance.addAllowedRolesToTokens(tokenAddresses, roles);
        compliance.addRoleToAccount(allowedRole, offeringAddress);

        // Approve offering to transfer up to the number of tokens for sale
        IERC20(assetToken).safeApprove(
            offeringAddress,
            IOffering(offeringAddress).numTokensForSale()
        );
    }

Ensure zero address check in the case the offering is 0 due to deletion or not existing offering. 

Remediation Plan

SOLVED : The Plural Energy team solved the issue by adding proper checks.

if (offeringAddress == address(0)) {
            revert OfferingNotFound(assetToken, offeringNum);
}

Most of the solidity for loops use an uint256 variable counter that increments by 1 and starts at 0. These increments don't need to be checked for over/underflow because the variable will never reach the max capacity of uint256 as it would run out of gas long before that happens.

It is recommended to uncheck the increments in for loops to save gas. For example, instead of:

for (uint i = 0; i < allowedTokenRolesLength; i++) {
            byteRoles[i + 1] = compliance.tokenAllowedRoles(assetToken, i);
}

Use:

for (uint i = 0; i < allowedTokenRolesLength;) {
            byteRoles[i + 1] = compliance.tokenAllowedRoles(assetToken, i);
            unchecked {++i};
}

Remediation Plan

ACKNOWLEDGED : The Plural Energy team acknowledged the issue.