Biconomy Hyphen 2.0 contest - peritoflores's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/security/Pausable.sol#L65-L68

Vulnerability details

Impact

If pauser makes an error all the protocol will be unusable

Proof of Concept

The funtion renouncePauser()#Pausable.sol which affect several contracts is dangerous.

function renouncePauser() external virtual onlyPauser {
    emit PauserChanged(_pauser, address(0));
    _pauser = address(0);
}

I believe that your implementation is similar to the deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v2.3.0/contracts/access/roles/PauserRole.sol

However, in your case pauser can break (accidentally of maliciously) the protocol by calling

1. pause
2. renouncePauser

and the protocol will be paused forever an not even the admin could unlock it.

Tools Used

Manual code review

Recommended Mitigation Steps

Maybe you could make admin able to set a new pauser. The same as OZ does in its new implementation allow the admin to set/ remove that role. I believe that this is the reason why the made that upgrade.

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L119-L121 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L284

Vulnerability details

Impact

Executors can, unintentionally, send a huge amount of ETH

Proof of Concept

The function setBaseGas(uint128 gas)#LiquidityPool.sol should have bounds like MAXBASEGAS. Even worse this function lacks of event emit.

As a result, executors can call sendFundsToUser and send enormous amount of ETH.

Tools Used

Manual code review

Recommended Mitigation Steps

Add bound checks and add an event emit

Gas Optimization for Biconomy by PeritoFlores

REVERT STRINGS LONGER THAN 32 BYTES

Some strings do not use custom errors and some of them are longer than 32 bytes Considering shortening them.

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/security/Pausable.sol#L43 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/ExecutorManager.sol#L17 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L77 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/LPToken.sol#L70

UNNECESSARY REQUIRE

There is an unnecesary require at

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L253

require(msg.value != 0, "Amount cannot be 0");

This is unnecesary because two lines before you check

require(
        tokenManager.getDepositConfig(toChainId, NATIVE).min <= msg.value &&
            tokenManager.getDepositConfig(toChainId, NATIVE).max >= msg.value,
        "Deposit amount not in Cap limit"
    );

 

Unless tokenManager.getDepositConfig(toChainId, NATIVE).min==0 and msg.value==0 the above line is a tautology