code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1061-L1075

Vulnerability details

Impact

Say a user has flashloaned and called stake function with N amount of PAL tokens. Due to minting mechanism, PAL tokens will not be transferred but the function will mint hPAL tokens to the user. And after cooldown period, user can call unstake and have the privilage to have N amount of PAL tokens transferred.

Proof of Concept

    function _stake(address user, uint256 amount) internal returns(uint256) {
        require(amount > 0, "hPAL: Null amount");

        // No need to update user rewards here since the _mint() method will trigger _beforeTokenTransfer()
        // Same for the Cooldown update, as it will be handled by _beforeTokenTransfer()    

        _mint(user, amount); //We mint hPAL 1:1 with PAL

        // Pull the PAL into this contract
        pal.safeTransferFrom(user, address(this), amount);

        emit Stake(user, amount);

        return amount;
    }

https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L1061-L1075

Tools Used

Manual Review

Recommended Mitigation Steps

pal.safeTransferFrom(user, address(this), amount) should be called upon success of _mint(user, amount)

Kogaroshi commented 2 years ago

Cannot reproduce the issue

Tests & fuzzing around that method do not reproduce that type of issue

In the _stake() method, the use of SafeTransferFrom() handles the scenario where the user has no funds, and will revert if the transfer cannot be made

In the case of a flashloan of PAL, the amount X of PAL to be staked will be transfered to the contract while minting the hPAL.

The recommended mitigation does not match the explained issue, and is not compatible with OZ ERC20.sol, as the -mint() method does not return a success bool.

Please provide more information & POC for this Issue

0xean commented 2 years ago

Closing as invalid.

. Due to minting mechanism, PAL tokens will not be transferred but the function will mint hPAL tokens to the user

Issue isn't clear on how the PAL token would not be transferred to the user.

code-423n4 / 2022-03-paladin-findings