search

sherlock-audit / 2022-10-merit-circle-judging

1 stars 0 forks source link

ctf_sec - Give _escrowPool max allowance is risky in BasePool.sol#__BasePool_init #7

Closed sherlock-admin closed 2 years ago

sherlock-admin commented 2 years ago

ctf_sec

medium

Give _escrowPool max allowance is risky in BasePool.sol#__BasePool_init

Summary

Give _escrowPool max allowance is risky in BasePool.sol#__BasePool_init

Vulnerability Detail

Give _escrowPool max allowance is risky in BasePool.sol#__BasePool_init

    if(_rewardToken != address(0) && _escrowPool != address(0)) {
        IERC20(_rewardToken).safeApprove(_escrowPool, type(uint256).max);
    }

Given that _escrowPool is another upgradeable TimeLockPool, if the underlying smart contract is compromised, the malicious contract can drain the fund from the original TimeLockPool.

Source: https://kalis.me/unlimited-erc20-allowances/

Why are unlimited ERC20 allowances harmful? When depositing a specific amount (say 100 DAI) into a contract, you can choose to set an allowance of exactly that amount. But instead, many apps instead request an unlimited allowance from the user.

This offers a superior user experience because the user does not need to approve a new allowance every time they want to deposit tokens. By setting up an unlimited allowance, the user just needs to approve it once, and not repeat the process for subsequent deposits.

However, this setup comes with significant drawbacks. As we know, bugs can exist and exploits can happen even in established projects. And by giving these platforms an unlimited allowance, you do not only expose your deposited funds to these risks, but also the tokens that you're holding "safely" in your wallet.

Impact

if the underlying smart contract is compromised, the malicious contract can drain the fund from the original TimeLockPool.

Recently, a lot user suffers from TRANSIT SWAP hack because the TRANSIT SWAP contract is compromised and any user that does not revoke approval lose their fund

Source: https://rekt.news/transit-swap-rekt/

Code Snippet

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/base/BasePool.sol#L74-L77

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/base/BasePool.sol#L104-L108

Tool used

Manual Review

Recommendation

We recommend the project only give a least of allowance needed for the underlying contract to do the job

    function claimRewards(address _receiver) external {
        uint256 rewardAmount = _prepareCollect(_msgSender());
        uint256 escrowedRewardAmount = rewardAmount * escrowPortion / 1e18;
        uint256 nonEscrowedRewardAmount = rewardAmount - escrowedRewardAmount;

        if(escrowedRewardAmount != 0 && address(escrowPool) != address(0)) {
            IERC20(_rewardToken).safeApprove(_escrowPool, escrowedRewardAmount); // allowance is given here
            escrowPool.deposit(escrowedRewardAmount, escrowDuration, _receiver);
        }