bug: first depositor to a gate can front-run

[tserg]

The Gate module is a simplified implementation of ERC-4626, and there is a known vulnerability with the first issuance of shares (yang in Aura's case) when the total supply of shares is 0. See this video by Spearbit on their audit of Maple Finance.

This is how an attack would happen assuming the token is WETH, as replicated from the video:

• Attacker deposits 1 wei of WETH for 1 share. 1 share = 1 wei
• Attacker transfers 100 WETH to the contract. 1 share = 100 WETH + 1 wei
• User deposits 200 WETH to the contract. Due to integer division, user receives 1 share (rounded down from 1.999...). 2 shares = 300 WETH + 1 wei
• Attacker withdraws 1 share, amounting to 150 WETH, and profits 50 WETH - 1 wei.

Recommended solution:

• Mint a minimum amount of shares (e.g. 1,000) to an inaccessible address so that the cost of attack becomes prohibitively large. If there is always 1,000 shares, then the amount of WETH needed to perform the same attack in the example increases by 1000x.
  • The video recommended the zero address. This should work once fees are implemented on Starknet so a contract cannot be called directly.
• This also means the first depositor suffers a negligible loss.

[milancermak]

Related: transmissions11/solmate#178

[milancermak]

Related: tweet

[tserg]

Related: tweet

I am hesitant on the proposed mitigation by OZ because it introduces additional decimal precision. Also, it would affect all depositors, instead of the first depositor.

Regarding the fix to require a minimum initial deposit, his criticism was that this value is arbitrary and opinionated. While that may be true, I am not sure if it is a sufficiently strong consideration.

[0xBRM]

min deposit amount per token and make it sane? We run into this problem w sandclock and a single lusd deposit was enough to make the attack prohibitively expensive iirc. Agreed that we shouldn't care about it being opinionated.

[milancermak]

I'm in the minimum initial deposit camp as well. It's such a small amount I'd even require it in the constructor so we bear the burden. WDYT?

[tserg]

I'm in the minimum initial deposit camp as well. It's such a small amount I'd even require it in the constructor so we bear the burden. WDYT?

I think that is a good idea. We can have the minimum initial deposit set as a constructor argument so that we can account for non-18 decimals like WBTC at deployment, and since the minimum initial deposit is already transferred, we do not need to change the existing code outside of the constructor.

[tserg]

I realized there are two obstacles to implementing the transfer of the initial amount in the constructor in our current design.

1. Other than transferring the initial amount of the asset, we also need to mint the initial amount of yang. However, the yang supply is a storage variable in Shrine, and the Gate reads this value from the Shrine. Since only the Abbot is authorized to call Shrine.deposit to mint yang in the Shrine, we are unable to update the yang supply in Shrine within the constructor of the Gate.
2. Yang needs to be owned by a trove. If the initial amount if provided by us in the constructor, we could assign it to a trove ID for free, but it would also mean that trove is potentially liable to receive redistributions.
3. Even if (1) and (2) are overcome, we would still need to change the code for deposit to account for the initial shares if all the existing yang were withdrawn. This is to catch the scenario where (1) gate is deployed with initial amount provided by us, and assigned to a trove; (2) all yang is withdrawn; (3) attacker deposits 1 wei.

Since we would still need to do (3) even if we provide the initial deposit in the constructor, then it seems cleaner to leave it out of the constructor.

[tserg]

This is more complex than I originally thought. If we are unable to assign the yang amount corresponding to the minimum initial deposit to an inaccessible address, then the usual method of requiring a minimum initial deposit would not be sufficient to protect against this exploit. A malicious user can deposit the minimum (e.g. 1000 wei) then withdraw in full except 1 wei.

I am now of the opinion that the constructor approach is better with the following addition:

• Set the initial supply to the minimum initial deposit when calling Shrine.add_yang, to correspond with the initial minimum deposit amount in the constructor for Gate.

This will allow us to mint the yang for the initial minimum deposit, without assigning it to a trove and exposing it to redistributions.

[milancermak]

So in Shrine.add_yang you'd do a "fake mint"?

[tserg]

So in Shrine.add_yang you'd do a "fake mint"?

After some tinkering around, I think we can do a real mint in a less straightforward manner instead of performing the token transfer in the constructor of Gate:

1. In Sentinel.add_yang, we perform the transfer of the initial deposit (defined as a constant in Sentinel) to the gate by calling ERC20.transferFrom(caller, gate, INITIAL_AMOUNT), then call Shrine.add_yang.
2. We then additionally pass the initial deposit amount (initial_yang_amt) to Shrine.add_yang, which sets the total supply to initial_yang_amt.

Since a deployed Gate is unusable until it is properly added via Sentinel.add_yang, we can safely delay the minting of the initial shares until this step.

This allows us to (1) properly mint the yang for the equivalent token balance in the gate; (2) not subject any trove to a dust amoutn of yang, and therefore susceptible to redistribution; (3) does not require any changes to existing authorizations.

[milancermak]

That looks like a solid solution 👍