Zap contract's mint() allows minting ibbtc tokens for free

[code423n4]

Handle

Ruhum

Vulnerability details

Impact

The user can call the mint() function with any contract address that implements the safeTransferFrom() function. Thus, they can mint as many ibbtc tokens for free as they want.

Proof of Concept

https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L100

Technically, they can deploy a contract that has a safeTransferFrom() function that simply returns true. They then call the mint() function and pass the contract address and an arbitrary value for the amount parameter. The mint() function will then go ahead and mint them the passed amount of ibbtc tokens without receiving anything in return.

There are no checks that verify that the passed token address is either the wBTC or renBTC token.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add the following check to the beginning of the function:

require(token == address(ren) || token == address(wbtc);

[tabshaikh]

We should totally add the require(token == address(ren) || token == address(wbtc); as a best practice. Disagree on the risk as users would not be able to mint ibbtc for free as firstly the contract is to hold no token and this will revert https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L129 if ren/ wbtc amount is not present

[GalloDaSballo]

Disagree with the finding, the finding claims that a user would be able to mint ibBTC with any token, this is not correct.

Using any token beside the supported ones, would cause a revert when adding liquidity to the pool or when we deposit in the yearn vault.

See code for revert:
https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L104
https://github.com/Badger-Finance/ibbtc/blob/d8b95e8d145eb196ba20033267a9ba43a17be02c/contracts/Zap.sol#L108

[0xleastwood]

agree with sponsor, user's tx will revert if _addLiquidity or deposit is called. The two functions will attempt to pull the correct amount of funds from Zap.sol which would be 0 as in the warden's suggested attack vector.

[0xleastwood]

Will keep as low risk as it is useful to check token is either address(ren) or address(wbtc).