bRemBadger.MD

Refactor

Total Deposited Claimed

-> New Claim

MAX Claim -> Total Deposited - Claimed

The vault can be massively simplified to: -> Deposit Shares -> Store in storage -> Unlocks are calculated based on Total Deposited - Claimed -> No ERC20, no transfers, no shares math

Critical

Deposit + transfer allows early unlocks

-> Deposit for myself -> Get X per week -> Transfer to someone else -> Get X per week on that account

-> Allows burning at the same rate on multiple accounts -> Repeat this 12 times -> You will unlock all tokens on the first week

MED

Not Donation Resistant!

https://github.com/Badger-Finance/remBadgerVault/blob/9d83fb0bc0f36d54ce20736c2b6999c858bd8ac6/src/bremBadger.sol#L172-L177

    function getPricePerFullShare() public view returns (uint256) {
        if (totalSupply() == 0) {
            return 1e18;
        }
        return REM_BADGER_TOKEN.balanceOf(address(this)) * 1e18 / totalSupply();
    }

People can mint a small amount, then donate to cause inflation attack

This will allow a whale to farm the % of tokens from other depositors

It may also cause the very small per week precision loss to be magnified

Fake fungibility

Token has transfer function, but claims based on balance will work only at the end of vesting

Should this vault be a token? Or should it just be a deposit contract?

QA

Very small Precision loss per week

https://github.com/Badger-Finance/remBadgerVault/blob/9d83fb0bc0f36d54ce20736c2b6999c858bd8ac6/src/bremBadger.sol#L100

sharesPerWeek[msg.sender] = balanceOf(msg.sender) / VESTING_WEEKS;

Q

How are rewards distributed?

Are you just going to stream rewards externally?

Is remBADGER supposed to compound?

There's extra math to account for ppfs math, but is this necessary?

If rewards are streamed outside of the contract, shouldn't the withdrawal logic only be based on shares?

Deposits are shares the user deposited, but this seems wholly unncessary