code-423n4 / 2023-07-tapioca-findings

15 stars 10 forks source link

Incorrect parameter for allowedBorrow when repaying #1673

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L268

Vulnerability details

Impact

Incorrect parameter for allowedBorrow check during repayment in BigBang requires an approval that is orders of magnitudes higher than the intended amount (if Alice wants to allow Bob to use their funds). This can be abused by Bob to take more collateral or borrow more USDO on Alices behalf than what she intended.

Proof of Concept

The function _allowedBorrow is used to check if an address has given another address the allowance to spend its tokens.It expects a share value as second argument:

function _allowedBorrow(address from, uint share) internal {
    if (from != msg.sender) {
        if (allowanceBorrow[from][msg.sender] < share) {
            revert NotApproved(from, msg.sender);
        }
        allowanceBorrow[from][msg.sender] -= share;

Looking at the different usages in BigBang it is evident that this is meant to refer to a value expressed in userCollateralShare, for example in addCollateral:

function addCollateral(
    ...
    uint256 share
) public allowedBorrow(from, share) notPaused {
    _addCollateral(from, to, skim, amount, share);
}

function _addCollateral(
    ...
    uint256 share
) internal {
    if (share == 0) {
        share = yieldBox.toShare(collateralId, amount, false);
    }
    userCollateralShare[to] += share;

The usage of _allowedBorrow is consistent throughout the BigBang contract, with the exception of repay:

/// @param part The amount to repay. See `userBorrowPart`
function repay(
    ...
    uint256 part
) public notPaused allowedBorrow(from, part) returns (uint256 amount) {

Here the second argument part refers to a users debt expressed in userBorrowPart. Since user userCollateralShare and userBorrowPart have different underlying assets, they will also differ in their order of magnitude. For example Alice wants to allow Bob to repay 100 "parts" of his debt via her assets. This would be roughly 100 USDO (depending on accrued interest). She now gives an allowance of 100e18 to Bob. But this would also allow Bob to use 100e18 worth of Alices userCollateralShare in other actions, which could for example be ETH.

Tools Used

Manual Review

Recommended Mitigation Steps

The borrow function converts the USDO amount into an amount expressed in userCollateralShare:

uint256 allowanceShare = _computeAllowanceAmountInAsset(
    from,
    exchangeRate,
    amount,
    asset.safeDecimals()
);
_allowedBorrow(from, allowanceShare);

This logic should be used here as well.

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #578

c4-judge commented 1 year ago

dmvt marked the issue as satisfactory