code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/zaps/PoolMigrationZap.sol#L52-L58 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/VestedEscrow.sol#L102-L103 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L227-L230

Vulnerability details

Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance.

Rebasing tokens are tokens where, over time, any holder of the token has his/her balanceOf() grow, as a form of rewards

Note that even though a token may not currently charge a fee or be rebasing, a future upgrade to the token may institute one or cause it to become one.

Impact

If there is only one user that has deposited a fee-on-transfer token, that user will be unable to withdraw their funds, because the amount available will be less than the amount stated during deposit, and therefore the token's transfer() call will revert during withdrawal. For more users, consider what happens if the token has a 10% fee-on-transfer fee - deposits will be underfunded by 10%, and the users trying to withdraw the last 10% of deposits/rewards will have their calls revert due to the contract not holding enough tokens. If a whale does a large withdrawal, the extra 10% that that whale gets will mean that many users will not be able to withdraw anything at all.

For rebasing tokens, by the time the user withdraws, the balance will have grown and the user will miss out on the extra rewards, with the contract locking them

Proof of Concept

There are multiple places where the contracts involved don't use the actual amount that ends up in its balance:

In migrate(), redeem() states the amount it transfers out, which will be wrong for fee-on-transfer tokens, and then migrate() uses that amount in a call to depositFor(), which will fail because the PoolMigrationZap does not hold enough funds:
```
File: protocol/contracts/zaps/PoolMigrationZap.sol   #1
```

59 uint256 underlyingAmount = oldPool.redeem(lpTokenAmount); 60 address underlying = oldPool.getUnderlying(); 61 ILiquidityPool newPool = underlyingNewPools[underlying]; 62 uint256 ethValue = underlying == address(0) ? underlyingAmount : 0; 63 newPool.depositFor{value: ethValue}(msg.sender, underlyingAmount);

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/zaps/PoolMigrationZap.sol#L59-L63

2. In `fund()`, the amount stated is incorrectly sored as the amount the contract holds, rather than the balance after the transfer:
```solidity
File: protocol/contracts/tokenomics/VestedEscrow.sol   #2

102               rewardToken.safeTransfer(holdingAddress, amount);
103               initialLocked[recipient_] = initialLocked[recipient_] + amount;

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/VestedEscrow.sol#L102-L103

Later when the user calls claim(), that function uses the function below to determine how much is owed, and because it uses the stored amount, the user will get more or less than is actually owed, or the call may revert:

File: protocol/contracts/tokenomics/VestedEscrow.sol   #3

159       function _totalVestedOf(address _recipient, uint256 _time) internal view returns (uint256) {
160           return _computeVestedAmount(initialLocked[_recipient], _time);
161       }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/VestedEscrow.sol#L159-L161

In lockFor() the amount passed in is stored rather than the balance after the transfer:
```
File: protocol/contracts/BkdLocker.sol   #4
```

227 function lockFor(address user, uint256 amount) public override { 228 govToken.safeTransferFrom(msg.sender, address(this), amount); 229 _userCheckpoint(user, amount, balances[user] + amount); 230 totalLocked += amount;

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L227-L230

Later when the user calls `prepareUnlock()`, it only verifies that the amount is within the balance stored by the above call to `_userCheckpoint()`:
```solidity
File: protocol/contracts/BkdLocker.sol   #5

118       function prepareUnlock(uint256 amount) external override {
119           require(
120               totalStashed[msg.sender] + amount <= balances[msg.sender],
121               "Amount exceeds locked balance"
122           );
123           totalStashed[msg.sender] += amount;
124           stashedGovTokens[msg.sender].push(
125               WithdrawStash(block.timestamp + currentUInts256[_WITHDRAW_DELAY], amount)
126           );
127           emit WithdrawPrepared(msg.sender, amount);
128       }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L118-L128

Tools Used

Code inspection

Recommended Mitigation Steps

Measure the contract balance before and after the call to transferFrom(), and use the difference between the two as the amount, rather than the amount stated

chase-manning commented 2 years ago

We don't support fee-on-transfer style tokens

GalloDaSballo commented 2 years ago

Because both BDK and the AMM tokens are known to be non-rebasing, the finding is highly unlikely to happen

Due to the reduced probability, as well as the clearly defined target tokens, I believe QA to be a more appropriate severity

code-423n4 / 2022-05-backd-findings