An early depositor to yVault.sol, preferably the first to deposit, will have the ability to steal funds from subsequent user deposits. The malicious user is able to do this by directly transferring tokens to either the yVault or Controller contracts following their deposit. If subsequent user deposits are less than the amount of token sent directly to the contracts, they will lose funds because their tokens will successfully transfer to the contract while they will be minted 0 shares
Proof of Concept
Steps to exploit:
Malicious User A deposits 1 wei of token. User A receives 1 share since supply is currently 0 based on the following clause.
if (supply == 0) {
shares = _amount;
Malicious User A then directly sends token to yVault.sol which will inflate the balance of the contract. For this example, User A sends 10,000 ether of token to the contract.
The next user B attempts to deposit 5,000 ether of token. Based on the following formula, they will receive 0 shares even though the funds have already been transferred to the contract.
Where balanceBefore is equal to balance() which is equal to token.balanceOf(address(this)) + controller.balanceOf(address(token)) The formula for the example looks like:
Malicious User A then calls withdraw(). Since the contracts hold 15,000 ether of tokens and User A holds the only share, they receive all 15,000 ether.
Tools Used
Manual review.
Recommended Mitigation Steps
Instead of relying on the balance of tokens of the two contracts, use internal accounting variables to represent the current balance of tokens that have been properly deposited and withdrawn from the vault. This will remove the ability to directly transfer tokens to the contracts to alter the share distribution logic.
spaghettieth
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L142-L157
Vulnerability details
Impact
An early depositor to
yVault.sol, preferably the first to deposit, will have the ability to steal funds from subsequent user deposits. The malicious user is able to do this by directly transferring tokens to either the yVault or Controller contracts following their deposit. If subsequent user deposits are less than the amount of token sent directly to the contracts, they will lose funds because their tokens will successfully transfer to the contract while they will be minted0 sharesProof of Concept
Steps to exploit:
1 weiof token. User A receives 1 share since supply is currently 0 based on the following clause.yVault.solwhich will inflate the balance of the contract. For this example, User A sends10,000 etherof token to the contract.5,000 etherof token. Based on the following formula, they will receive0 shareseven though the funds have already been transferred to the contract.Where
balanceBeforeis equal tobalance()which is equal totoken.balanceOf(address(this)) + controller.balanceOf(address(token))The formula for the example looks like:shares = (5,000 ether * 1 share) / 10,000 ether = 0 shareswithdraw(). Since the contracts hold15,000 etherof tokens and User A holds the only share, they receive all 15,000 ether.Tools Used
Manual review.
Recommended Mitigation Steps
Instead of relying on the balance of tokens of the two contracts, use internal accounting variables to represent the current balance of tokens that have been properly deposited and withdrawn from the vault. This will remove the ability to directly transfer tokens to the contracts to alter the share distribution logic.