Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Loss of funds when creating offer

holydevoti0n

Summary

When creating an offer, there is no validation to check whether the msg.valuepassed in == to the amount expected.

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L39-L157

@> function createOffer(CreateOfferParams calldata params) external payable {
{
/// @dev transfer collateral from _msgSender() to capital pool
uint256 transferAmount = OfferLibraries.getDepositAmount(
params.offerType,
params.collateralRate,
params.amount,
true,
Math.Rounding.Ceil
);
​
​
ITokenManager tokenManager = tadleFactory.getTokenManager();
@> tokenManager.tillIn{value: msg.value}(
_msgSender(),
params.tokenAddress,
transferAmount,
false
);
}
​
}

See that the transferAmountis calculated based on the params.amount not the msg.value.

In the TokenManagerthe validation is based on the transferAmount:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L86

if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @notice check msg value
* @dev if msg value is less than _amount, revert
* @dev wrap native token and transfer to capital pool
*/
@> if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
}

Due to this flat verification where msg.valueand transferAmountrepresent different values, the TokenManagerwill receive the ETH sent from the PreMarketand only account for the amount from params.amount.

Thus, any additional amount sent as msg.valuewill be lost and retained in the TokenManagercontract.

I.e: user msg.value is 100ETH but params.amount is only 1 ETH. When the user calls withdraw, he only receives 1 ETH and loses the 99 ETH sent to the protocol.

PoC

In order to run this PoC, add the fix for the DoS on withdraw. Submission: "DoS on withdraw due to wrong parameter on approve call"

Then add the following test on PreMarkets.t.soland run forge test --match-test test_whenUserCreateOffer_heWillLoseFunds -vv

function test_whenUserCreateOffer_heWillLoseFunds() public {
// pre condition - user deposit is 1ETH but protocol receives 100ETH
vm.startPrank(user1);
// balance of user before deposit - 100ETH
deal(user1, 100e18);
preMarktes.createOffer{value: 100e18}( // @audit user send 100 ETH
CreateOfferParams(
marketPlace,
address(weth9),
1000,
1e18, // @audit protocol receives 1 ETH
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
console2.log("user balance after createOffer %e", user1.balance);
​
address stock1Addr = GenerateAddress.generateStockAddress(0);
address offer1Addr = GenerateAddress.generateOfferAddress(0);
preMarktes.closeOffer(stock1Addr, offer1Addr);
​
// act: withdraw
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
​
// post condition: user has lost most of his funds.
console2.log("user balance after withdraw %e", user1.balance);
console2.log("tokenManager balance %e", address(tokenManager).balance);
​
vm.stopPrank();
}

Output:

Ran 1 test for test/PreMarkets.t.sol:PreMarketsTest
[PASS] test_whenUserCreateOffer_heWillLoseFunds() (gas: 680013)
Logs:
user balance after createOffer 0e0
@> user balance after withdraw 1.2e18
@> tokenManager balance 9.88e19
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.00ms (1.51ms CPU time)

Impact

Loss of funds when creating offer.

Tools Used

Manual Review & Foundry

Recommendations

  1. Check if msg.value== transferAmount. If false, revert.

  2. (Alternative) At the end of the transaction return any remaining ETH to the user.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-TokenManager-tillin-excess

Invalid, these are by default, invalid based on codehawks [general guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). The check implemented is simply a sufficiency check, it is users responsibility to only send an appropriate amount of native tokens where amount == msg.value when native token is intended to be used as collateral (which will subsequently be deposited as wrapped token). All excess ETH can be rescued using the `Rescuable.sol` contract. > Users sending ETH/native tokens > If contracts allow users to send tokens acc111identally.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.