c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L524-L532

Vulnerability details

Impact

Users may transfer more money than intended, resulting in a net loss for the user.

Proof of Concept

In _bridgeOut, either the hToken or the underlying token is being sent to the BranchPort address. For the hToken, the tokens will be burned and for the underlying, the token will be stored in the BranchPort contract.

        // Check if hTokens are being bridged out
        if (_hTokenAmount > 0) {
            _localAddress.safeTransferFrom(_depositor, address(this), _hTokenAmount);
            ERC20hTokenBranch(_localAddress).burn(_hTokenAmount);
        }

        // Check if underlying tokens are being bridged out
        if (_deposit > 0) {
            _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

According to the discussions on Discord, protocol admin mentions that if the user has 5 hToken and 5 underlying token, the amount called should be 10 and _deposit should be 5. This means that 5 hToken will be burnt from the user and 10 underlying token will be transferred from the user to the Port, instead of 5 underlying token.

If a user wants to transfer 10 tokens in total, he has to transfer 15 instead (5 hToken burnt and 10 underlying transferred)

Tools Used

VSCode

Recommended Mitigation Steps

Make sure that the calculation is correct so that if the user wants to transfer 10 tokens (5 htoken and 5 underlying), he will only need 5 hToken and 5 underlying, instead of 5hToken and 10 underlying. This issue might be due to this line

        uint256 _hTokenAmount = _amount - _deposit;

which is pretty confusing. Amount should not be the total amount of tokens to bridge out but rather just the underlying amount of tokens to bridge out.

Assessed type

Other

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

0xA5DF commented 1 year ago

This means that 5 hToken will be burnt from the user and 10 underlying token will be transferred from the user to the Port, instead of 5 underlying token.

No, user would be paying with 5 hTokens and 5 underlying tokens. So user gets what he paid for. Any suggestion regarding the way parameters are named isn't more than a QA

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)