Potential Over-redemption Vulnerability in redeem Function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L152-L153

Vulnerability details

Impact

In the redeem function, when a third party is using their allowance to redeem shares on behalf of an owner, there exists a potential scenario where the third party could redeem more than originally intended by the owner.

Proof of Concept

This is how the vulnerability could be exploited:

Alice allows Bob to redeem 100 shares on her behalf. Alice later decides to reduce Bob's allowance to 50 shares, so she sends a transaction to do this. Bob, upon noticing Alice's transaction in the mempool, quickly sends a transaction to redeem the original 100 shares. Due to factors like higher gas fees or quicker block inclusion, Bob's transaction gets confirmed first. His allowance is reduced by 100 shares. Now, Alice's transaction (which intended to set Bob's allowance to 50) gets confirmed. Bob can now redeem an additional 50 shares. In total, Bob redeems 150 shares, even though Alice intended for him to redeem at most 100.

The above situation arises due to the fact that the allowance check and deduction are separate from the action of reducing the allowance. This asynchronous nature can be manipulated when transactions are processed out of their intended order.

Tools Used

foundry

Recommended Mitigation Steps

Immediate Fix: Implement checks that prevent modifying the allowance of a third party if they still have a non-zero allowance. Owners would have to set an allowance to 0 before setting a new positive allowance. This is a common pattern seen in some ERC20 contracts.

Structural Fix: Refactor the allowance mechanism to use a more robust pattern similar to the increaseAllowance and decreaseAllowance methods seen in many well-audited ERC20 contracts. When decreasing allowance, ensure that the deduction doesn't underflow the current allowance, and when increasing allowance, ensure it doesn't overflow.

Update the redeem function to first check the current allowance and then set it to zero, before adjusting it.

function redeem(
    uint256 shares,
    address receiver,
    address owner
) public returns (uint256 assets, uint256 rdpxAmount) {
    perpetualAtlanticVault.updateFunding();

    if (msg.sender != owner) {
        uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

        require(allowed >= shares, "Allowance exceeded"); // Ensure there's enough allowance

        // If allowed is not max, then set the allowance to zero
        if (allowed != type(uint256).max) {
            allowance[owner][msg.sender] = 0; // Reset allowance to zero
        }
    }

    (assets, rdpxAmount) = redeemPreview(shares);

    // Check for rounding error since we round down in previewRedeem.
    require(assets != 0, "ZERO_ASSETS");

    _rdpxCollateral -= rdpxAmount;

    beforeWithdraw(assets, shares);

    _burn(owner, shares);

    collateral.transfer(receiver, assets);

    IERC20WithBurn(rdpx).safeTransfer(receiver, rdpxAmount);

    emit Withdraw(msg.sender, receiver, owner, assets, shares);
}

Add a new function that allows setting a new allowance only if the current allowance is zero:

function approve(address spender, uint256 amount) public override returns (bool) {
    require(amount == 0 || allowance[msg.sender][spender] == 0, "Approve: current allowance must be zero to set a new non-zero allowance");
    allowance[msg.sender][spender] = amount;
    emit Approval(msg.sender, spender, amount);
    return true;
}

With these modifications:

A user must first reset any existing allowance to zero before setting a new allowance. The redeem function ensures that if a spender has a non-maximum allowance, it gets reset to zero upon a successful redeem, ensuring that any subsequent transactions trying to use that allowance will fail.

The exploitation of this contract revolves around the asynchronous nature of blockchain transactions, where the order of transactions isn't guaranteed. Malicious actors can monitor the mempool for transactions and strategically place their own transactions to exploit this vulnerability.

Assessed type

Invalid Validation

[bytes032]

Invalid

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid