code-423n4 / 2024-05-olas-validation

0 stars 0 forks source link

unsafe approve would stop bridging of tokens #191

Open c4-bot-7 opened 4 months ago

c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/staking/ArbitrumDepositProcessorL1.sol#L174

Vulnerability details

Impact

Failure to bridge tokens to other contract of L2 from L1

Proof of Concept

Olas uses approve that do not handle non-standard erc20 behavior. Acc: to weird ERC20 repo

Some token contracts do not return any value. Some token contracts revert the transaction when the allowance is not zero.

One such instance from ArbitrumDepositProcessorL1

        if (transferAmount > 0) {
            // Approve tokens for the bridge contract
            IToken(olas).approve(l1ERC20Gateway, transferAmount);

If one such token is USDT[which doesn't return any value and has a approval race condition of not approving amt1 >0 when amt0>0 is already approved] / Tether Gold[returns false even if transfers are succesfull], so it may lead the user to believe that the tokens has not been approved and they may approve it again , sending double amount or end up reverting

Tools Used

None

Recommended Mitigation Steps

Use solmate safeApprove instead and set the allowance to 0 before calling it.

Note: The recommendation should be applied to all such instances{ 1, 2, 3}

Assessed type

ERC20

Saptarshi1010 commented 4 months ago

@0xA5DF not really escalating it, Just asking since ik this might have been reported in the findings bot, but since some reports still get validated due to severity, so kinda poping the question. Thanks

0xA5DF commented 4 months ago

If one such token is USDT

The token used in the code you referred to is Olas, not any type of token As long as Olas doesn't have this issue, this isn't relevant