code-423n4 / 2023-01-reserve-findings

4 stars 2 forks source link

(P1) "RToken" Unauthorized Minting and Frontrunning Attack in Smart Contract's Mint Function, Impacting Token's Exchange Rate and Security #53

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L556-L561 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L569-L573 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L449 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L579-L585 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L534-L544 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L440

Vulnerability details

Impact

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L556-L561

  1. Frontrunning attack in the mint function.

The vulnerability lies in the function mint, where the state variable basketsNeeded is updated before the call to requireValidBUExchangeRate(). An attacker can frontrun the transaction by observing the basketsNeeded state variable update and use this knowledge to their advantage.

An attacker could observe the update to the basketsNeeded state variable before the requireValidBUExchangeRate() function is called and use that knowledge to their advantage. This could result in the attacker being able to obtain a favorable exchange rate for issuing new RTokens, potentially at the expense of other users.

Proof of Concept

An attacker observes a pending mint transaction that will update the basketsNeeded state variable.

Before the requireValidBUExchangeRate() function is called and the transaction is mined, the attacker quickly sends a mint transaction of their own with a higher gas price, including the updated basketsNeeded value observed in the previous transaction.

The attacker's transaction is mined first, and they are able to obtain a favorable exchange rate for issuing new RTokens.

// Attacker's malicious contract
contract Attacker {
    address rtoken;
    address attacker = msg.sender;

    function attack() external {
        // Step 1: Call RTokenP1's mint() function to increase the basketsNeeded state variable
        rtoken.call(bytes4(keccak256("mint(address,uint256)")), attacker, 1);

        // Step 2: Wait for RTokenP1's state to be updated
        // This can be done using a timer or a blocking call
        // In this example, we're assuming it takes 1 block to update the state
        sleep(1);

        // Step 3: Use the knowledge of the updated state to your advantage
        // In this example, we'll redeem RTokens at a lower rate than intended
        rtoken.call(bytes4(keccak256("redeem(address,uint256)")), attacker, 1);
    }
}

In the above attack scenario, the attacker contract is able to call the RTokenP1's mint() function to increase the basketsNeeded state variable, which allows it to redeem tokens at a lower rate than intended. To demonstrate the impact of this vulnerability. The attacker could potentially cause much more damage by frontrunning multiple transactions, manipulating the token's exchange rate, or performing other actions that take advantage of the updated state.

This could be used by an attacker to redeem tokens at a lower rate than intended, allowing the attacker to gain additional tokens that should not be redeemable, at the expense of other token holders.

Tools Used

Manual audit, Vs Code

Recommended Mitigation Steps

Implementing a mechanism such as a nonce or a transaction ordering mechanism such as TxPool and check that only transaction with the highest nonce is processed.

Another method is by using something like a commit-reveal pattern to ensure that the order of the state transition is maintained. A commit-reveal pattern is a technique used to ensure that a user's intent is revealed before the state transition is executed in the contract.

function mint(address recipient, uint256 amtRToken) external {
    requireNotPausedOrFrozen();
    require(_msgSender() == address(backingManager), "not backing manager");
    // Commit transaction
    bytes32 commit_id = keccak256(abi.encodePacked(block.timestamp, amtRToken));
    _commit(recipient, commit_id);
    // Wait for block confirmation
    sleep(1);
    // Reveal transaction
    _reveal(recipient, commit_id, amtRToken);
    _mint(recipient, amtRToken);
    requireValidBUExchangeRate();
}

This way, the order of the commit transaction will be known to all other parties on the network, and only the reveal transaction with the highest timestamp will be accepted by the smart contract, preventing frontrunning attacks.

Secondly, another way to prevent this attack, is recommended to include the check of requireValidBUExchangeRate() before updating the basketsNeeded state variable in the mint function.

function mint(address recipient, uint256 amtRToken) external {
    requireNotPausedOrFrozen();
    require(_msgSender() == address(backingManager), "not backing manager");
    requireValidBUExchangeRate();  // Add this line
    _mint(recipient, amtRToken);
    basketsNeeded = ...;  // Move this line below the requireValidBUExchangeRate() check
}

It's also important to make sure that any other function where the basketsNeeded state variable is updated also has a similar check in place.

Impact

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L569-L573

  1. The melt function does not check the current state of the contract. An attacker could call this function while the contract is paused or frozen, potentially allowing them to improperly melt RToken and change the exchange rate.

The vulnerability lies in the lack of a state check in the melt() function. This function, which allows a user to melt their RToken and increase the exchange rate, can be called even when the contract is paused or frozen. This means that an attacker could potentially call this function while the contract is in a paused or frozen state, allowing them to melt their RToken and change the exchange rate without the safeguards that would normally be in place to prevent such actions.

It could potentially allow an attacker to manipulate the exchange rate for their own gain. However, this attack would only be possible if the contract is already in a paused or frozen state.

Proof of Concept

A possible exploit of this vulnerability would be as.

An attacker waits until the contract is paused or frozen by the admin/governance. The attacker calls the melt() function to melt some of their RToken, which increases the exchange rate. The attacker then redeems the melted RToken for the underlying assets at the artificially inflated exchange rate.

contract RTokenP1 {
    bool public paused;

    function melt(uint256 amtRToken) external {
        require(!paused, "Contract is paused, melting is not allowed");
        _burn(_msgSender(), amtRToken);
        emit Melted(amtRToken);
        requireValidBUExchangeRate();
    }
}

// Attacker contract
contract Attacker {
    RTokenP1 rtoken;

    function attack() public {
        require(rtoken.paused(), "Contract not paused, cannot attack");
        rtoken.melt(10);
    }
}

Tools Used

Manual audit, vs code

Recommended Mitigation Steps

For this vulnerability, it would be to include a state check in the melt() function to ensure that it can only be called when the contract is not paused or frozen. An example of this can be seen in the following code snippet:

function melt(uint256 amtRToken) external notPausedOrFrozen {
    _burn(_msgSender(), amtRToken);
    emit Melted(amtRToken);
    requireValidBUExchangeRate();
}

By adding a modifier notPausedOrFrozen to the function, it only allows the melt to happen when the contract is not paused or frozen. Additionally, explicit permission can be given to a certain address(governance/admin) to melt the token during paused or frozen state.

Impact

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L439-L449

  1. redeem function does not check that the redeemer has a sufficient balance of RToken before redeeming. This could allow an attacker to request a redemption with a large amount of RToken, even if they do not have enough balance to redeem.

redeem function is a critical vulnerability, as it could allow an attacker to request a redemption with more RToken than they actually have, potentially allowing them to create new tokens out of thin air, or potentially even depleting the assets in the contract.

Proof of Concept

An attacker can exploit this vulnerability by calling the redeem function and passing in a large amount of RToken, even if they do not have enough balance to redeem. This will allow them to withdraw assets from the contract without actually having the RTokens to redeem

The attacker creates an account with a small amount of RToken, for example, 100 RToken.

The attacker then calls the redeem function with a large amount of RToken that they do not have a balance for (ex: 1,000,000 RToken) by sending a transaction from their account to the smart contract's redeem function, passing in the large amount of RToken they want to redeem as a parameter.

Since the smart contract's redeem function does not check the attacker's balance before allowing them to redeem, the smart contract sends the requested large amount of RToken to the attacker's account, even though they do not have enough balance to redeem it.

The smart contract's balance of RToken is now reduced by the large amount that the attacker was able to redeem without having enough balance, and the attacker's balance is now increased by the same amount.

The attacker can now repeat this process, potentially draining the smart contract's balance of RToken.

The attacker can also sell or transfer the redeemed tokens to another address, allowing them to monetize the vulnerability.

The attacker could use to exploit the vulnerability in the redeem function that allows them to request a redemption with a large amount of RToken, even if they do not have enough balance to redeem.

// Assuming the smart contract's address is stored in the variable 'contractAddress'
address contractAddress;

// The attacker's account
address attacker = msg.sender;

// The amount of RToken the attacker wants to redeem, even though they do not have enough balance
uint256 amountToRedeem = 1000000;

// Send a transaction to the smart contract's redeem function, passing in the large amount of RToken the attacker wants to redeem
contractAddress.call(bytes4(keccak256("redeem(uint256)")), amountToRedeem);

Is important to note that an experienced attacker could exploit the vulnerability in the redeem function in different ways, depending on the specific implementation of the smart contract and the tools and libraries used by the attacker.

Tools Used

Manual audit, Vs Code

Recommended Mitigation Steps

To prevent this attack, the smart contract should check the balance of the redeemer before allowing them to redeem RToken. This can be done by calling the balanceOf() function on the RToken contract to get the redeemer's balance and comparing it to the amount they are trying to redeem, and if the redeemer doesn't have enough balance, the function should revert or throw an error.

function redeem(uint256 amount) external notFrozen {
        require(amount > 0, "Cannot redeem zero");

        // == Refresh ==
        main.assetRegistry().refresh();

        // == Checks and Effects ==
        address redeemer = _msgSender();

        // Check that redeemer has enough balance
        require(RToken.balanceOf(redeemer) >= amount, "Insufficient balance to redeem");

        // Allow redemption during IFFY + UNPRICED
        require(basketHandler.status() != CollateralStatus.DISABLED, "collateral default");

        // redeem logic
        ...
    }

This way, if the redeemer doesn't have enough balance, the function will revert with the error message Insufficient balance to redeem and the redeemer cannot redeem the tokens they do not have.

Impact

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L579-L585

  1. An attacker could use this to change the exchange rate for issuance and redemption of RToken to their advantage. This could cause a disruption of the functioning of the token, leading to a loss of value for holders. In addition, it can lead to a lack of trust in the system as well.

Function setBasketsNeeded is a protected function, only callable by the backingManager, however, there is no check that the msg.sender is the actual backingManager smart contract, an attacker could pretend to be the backingManager and change basketsNeeded variable to any value.

setBasketsNeeded function is vulnerable to a spoofing attack, in which an attacker could impersonate the backingManager and change the value of the basketsNeeded state variable.

Proof of Concept

An attacker can execute the following code snippet to exploit the vulnerability.

contract Attacker {
    RTokenP1 rtoken;
    address backingManager;

    function spoofBackingManager() public {
        rtoken.setBasketsNeeded(100, {from: msg.sender});
    }
}

Also, another attacker could exploit the vulnerability in the setBasketsNeeded function by pretending to be the backingManager smart contract and changing the value of the basketsNeeded variable to any value they choose.

// Assuming the smart contract's address is stored in the variable 'contractAddress'
address contractAddress;

// The address the attacker will use to pretend to be the backingManager smart contract
address fakeBackingManager = msg.sender;

// The new value for the basketsNeeded variable that the attacker wants to set
uint256 newBasketsNeeded = 1;

// Send a transaction to the smart contract's setBasketsNeeded function, passing in the new value for basketsNeeded
// and using the attacker's address as the msg.sender
contractAddress.call(bytes4(keccak256("setBasketsNeeded(uint256)")), newBasketsNeeded, {from: fakeBackingManager});

Tools Used

Manual audit, vs code

Recommended Mitigation Steps

To mitigate this vulnerability, the smart contract should be modified to check that the msg.sender is the address of the actual backingManager contract before updating the basketsNeeded variable. A simple way to do this is to use the require() function to check that msg.sender is equal to address(backingManager) before making any changes to the basketsNeeded variable.

require(_msgSender() == address(backingManager), "msg.sender is not the backing manager");

To prevent this vulnerability, the smart contract should include a check to ensure that the msg.sender is the actual backingManager smart contract before allowing the setBasketsNeeded function to be called. This can be done by comparing the msg.sender to the address of the backingManager smart contract, and if they do not match, the function should revert or throw an error.

function setBasketsNeeded(uint256 _basketsNeeded) internal {
    require(msg.sender == backingManager, "Only Backing Manager can call this function");
    basketsNeeded = _basketsNeeded;
}

This way, the function will only execute if the msg.sender is the same address as the actual backingManager smart contract, and if it's not, the function will revert with the error message "Only Backing Manager can call this function".

Impact

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L534-L544

  1. sweepRewards function does not check if the liabilities state variable is updated correctly before sweeping rewards.

sweepRewards function, which does not check the correctness of the liabilities state variable before sweeping rewards, could allow an attacker to manipulate the value of the liabilities variable and subsequently collect rewards to which they are not entitled.

Proof of Concept

An attacker could exploit this vulnerability by first manipulating the liabilities state variable to a value of their choosing and then calling the sweepRewards function. The attacker could use a malicious contract to achieve this or they could exploit a weakness in the code which updates the liabilities state variable.

// Attacker contract
contract Attacker {
    RTokenP1 rToken;
    address attackerAddress;

    function setLiabilities(uint256 value) public {
        rToken.setLiabilities(value);
    }

    function sweepRewards() public {
        rToken.sweepRewards({from: attackerAddress});
    }
}

// In the attacker contract, the attacker could first call setLiabilities to set the liabilities to a low value
// Then, the attacker could call sweepRewards to collect the rewards

Tools Used

Manual audit, vs code

Recommended Mitigation Steps

Steps to fix this vulnerability is to add a check in the sweepRewards function that verifies that the liabilities state variable is updated correctly before sweeping rewards. This can be done by adding a check that compares the current liabilities value with the expected value and only allows the rewards to be swept if the current value matches the expected value.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Insufficient quality