Platform: Code4rena
Start Date: 22/04/2021
Pot Size: $120,000 USDC
Total HM: 41
Participants: 10
Period: 7 days
Judge: LSDan
Total Solo HM: 28
Id: 5
League: ETH
Rank: 8/10
Findings: 5
Award: $2,288.60
🌟 Selected for report: 3
🚀 Solo Findings: 4
🌟 Selected for report: toastedsteaksandwich
9.777 VETH - $508.40
0.2346 ETH - $586.62
toastedsteaksandwich
The Vether4.addExcluded() function on mainnet (0x4Ba6dDd7b89ed838FEd25d208D4f644106E34279) allows a user to exclude an address from transfer fees for a cost of 128 VETH. By exploiting the conditions in which fees are taken, it is possible to set up a contract for a once-off cost in which all users can use to avoid transfer fees.
All transfer fees can be avoided by routing transfers through an excluded contract. An estimated $140k of transfer fees was accumulated at the time of writing. These fees can be avoided in future, causing an indirect loss of funds for the contract.
I've listed the a test case and the transferForwarder contract source in the following gist: https://gist.github.com/toastedsteaksandwich/2057bfeca5f0340838970c7ee9c9d7ab
Hardhat with mainnet forking pinned to block 12227519
The _transfer() function should be updated to only exclude transfer fees if the sender has been excluded, not both the sender and the recipient. This would prevent any user from being able to set up a central transfer forwarder as demonstrated. Moreover, the Transfer(_from, address(this), _fee); event should only be emitted if the sender has been excluded from transfer fees.
🌟 Selected for report: toastedsteaksandwich
9.777 VETH - $508.40
0.2346 ETH - $586.62
toastedsteaksandwich
The USDV balance of the Vader contract is vulnerable to theft through the Vader.redeemToMember() function. A particular case is through USDV redemption front-running. Users can redeem USDV for Vader through the USDV.redeemForMember() function or the Vader.redeemToMember() function. In the case of Vader.redeemToMember(), a user would need to send their USDV to the contract before redemption. However, as this process does not happen in a single call, the victim's call is vulnerable to front running and could have their redeemed USDV stolen by an attacker.
User's redeem USDV could be stolen by an attacker front running their Vader.redeemToMember() call.
The steps are as follows:
Note that while this particular case is front running a redemption call, any USDV balance could be stolen in this manner. Please find the POC showing the above steps here: https://gist.github.com/toastedsteaksandwich/39bfed78b21d7e6c02fe13ea5b2023c3
Hardhat on a local hardhat network
The Vader.redeemToMember() function should be restricted so that only the USDV contract can call it. Moreover, the amount parameter from USDV.redeem() or USDV.redeemForMember() should also be passed to Vader.redeemToMember() to avoid the need to sweep the entire USDV balance. In this way, the member's redemption happens in a single tx, and would only be allocated as much Vader as redeemed in USDV.
#0 - strictly-scarce
2021-04-27T12:44:01Z
Vader complies with a monetary security policy of "money in, money out". Contracts will only send out funds if they are first sent funds.
This is the case for the entire system, not just Vader.redeemToMember(), such as swaps and adding liquidity. Vader is not designed to be interacted with directly, it should be wrapped. In this case, users should convert and redeem only thru the USDV contract, which first sends funds.
Incidentally this is the same mechanism that uniswap employs for withdrawing liquidity, or syncing funds to balances. You can also get front-runned if you do it in two tx, it should be wrapped in 1 tx.
#1 - Mervyn853
2021-05-01T08:44:15Z
Our decision matrix for severity:
0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.
Recommended: 0
🌟 Selected for report: toastedsteaksandwich
0.8799 VETH - $45.76
0.0211 ETH - $52.80
toastedsteaksandwich
Due to the implementation of the approve() function in Vader.sol, Vether.sol and mainnet Vether4 at 0x4Ba6dDd7b89ed838FEd25d208D4f644106E34279, it's possible for a user to over spend their allowance in certain situations.
The steps to the attack are as follows:
Overall, Bob was supposed to be approved for at most 5000 VETH but got 7500 VETH. The POC code can be found here: https://gist.github.com/toastedsteaksandwich/db32472ae5c19c2eb188f07abddd02fa
Note that in the POC, Bob receives 7492.5 VETH instead of 7500 VETH due to transfer fees.
Hardhat with mainnet forks, pinned to block 12227519.
Instead of having a direct setter for allowances, decreaseAllowance and increaseAllowance functions should be exposed which decreases and increases allowances for a recipient respectively. In this way, if the decreaseAllowance call is front-run, the call should revert as there is insufficient allowance to be decreased. This leaves Bob with at most 5000 VETH, the original allowance.
#0 - strictly-scarce
2021-04-27T12:51:39Z
This theoretical attack has been known about for a while but never actually observed meaningfully. Addressing it costs extra gas.