code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L373-L376 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L158-L164

Vulnerability details

Impact

OpenZeppelin’s implementation of safeApprove won't work if approved amount is not set to 0. so the logics will fail if code don't set it 0 first. Both VeToken and VE3Token use OpenZeppelin’s ERC20 implementation and VeAssetDepositor and Booster calls safeApprove() of that tokens multiple times and deposit() function.

Proof of Concept

This is where VeAssetDepositor and Booster and calls safeApprove:

        if (_stake) {
            //mint here and send to rewards on user behalf
            ITokenMinter(token).mint(address(this), _amount);
            address rewardContract = pool.veAssetRewards;
            IERC20(token).safeApprove(rewardContract, _amount);
            IRewards(rewardContract).stakeFor(msg.sender, _amount);
        } else {

        } else {
            //mint here
            ITokenMinter(minter).mint(address(this), _amount);
            //stake for msg.sender
            IERC20(minter).safeApprove(_stakeAddress, _amount);
            IRewards(_stakeAddress).stakeFor(msg.sender, _amount);
        }

which don't set it to 0 first so those logics(deposit()) will be broken if the current approve value don't be 0

Tools Used

VIM

Recommended Mitigation Steps

set approved amount to 0 first.

solvetony commented 2 years ago

Duplicate of #185 #6 #235

GalloDaSballo commented 2 years ago

stakeFor will transferFrom which uses the entire allowance, in lack of a demonstration of a revert, considering that this is the code powering Convex and Aura finance am marking this invalid

code-423n4 / 2022-05-vetoken-findings