prePO contest - adriro's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L58 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L76

Vulnerability details

Some ERC20 token implementations have a fee that is charged on each token transfer. This means that the transferred amount isn't exactly what the receiver will get. A call to IERC20(token).transfer(recipient, 100) with a fee-on-transfer (FOT) of 5% will entitle the recipient with 95 tokens.

The main issue then lies in that the protocol will pull funds from arbitrary ERC20 token implementation using transferFrom, assuming that the receiving amount matches the amount sent as the parameter to the transfer call, which is not always the case.

Impact

The main impact can be found in the deposit function of the Collateral contract. This function takes an amount parameter which is used to pull funds from the user (line 49). After that, it assumes that the received amount matches the sent parameter amount. The function will calculate fees and call the deposit hook using that value. It will mint and entitle the user with collateral tokens based on the wrong amount value (line 58).

Now, if the user decides to withdraw the collateral, it will call the withdraw function with the minted collateral tokens, those will be converted back to roughly the original amount (minus protocol fees) and that wrong value will be used again to transfer the tokens back to the user (line 76). This means that it will start consuming collateral from other user's funds available in the contract, causing loss of funds. Each deposit/withdraw cycle will cause a loss of about the same percentage amount of the FOT from the token.

Note that other side effects also include a bad deposit fee calculation (the fee is calculated as a percentage of the amount argument in line 46) which will end up being more than it should be (as it is calculated based on the amount without the transfer fee) causing more to be sent to the treasury. Also note that the recorded amount in the DepositRecord contract will also be wrong.

PoC

Assuming an ERC20 token that implements FOT with a 5% fee:

1. User calls Collateral.deposit(account, 100). 100 tokens are removed from the user's account but only 95 are "received" in the Collateral contract.
2. User is minted collateral tokens based on the 100 tokens (line 58).

Now that if the user decides to withdraw their funds, then:

3. Calls Collateral.withdraw with the minted collateral tokens, that will get converted to roughly 100 tokens (minus protocols fees) and the function will transfer that amount (line 76).

Recommendation

Improve support for FOT type of ERC20. When pulling funds from the user using transferFrom the usual approach is to compare balances pre/post transfer, like so:

uint256 balanceBefore = IERC20(token).balanceOf(address(this));
IERC20(token).transferFrom(msg.sender, address(this), amount);
uint256 transferred = IERC20(token).balanceOf(address(this)) - balanceBefore;
// use transferred instead of amount