search

code-423n4 / 2021-09-defiprotocol-findings

1 stars 0 forks source link

Unintentionally causing users to lose their bond #105

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

itsmeSTYJ

Vulnerability details

Impact

Normally I would classify this as low but because of how widespread USDT is, I believe it is important to handle this case simply for USDT. USDT doesn't allow you to approve allowance unless you first set it to 0.

If your basket contains USDT initially and the publisher tries to publishNewIndex with USDT (regardless of weighting), any bonder that tries to settle the auction will always fail because basket.setNewWeights will always fail when it comes to approving max allowance for USDT.

The publisher might not be malicious but the publisher not knowing about this is not an excuse for users losing their bond.

Recommended Mitigation Steps

function approveUnderlying(address spender) private {
    for (uint256 i = 0; i < weights.length; i++) {
                if(IERC20(tokens[i]).allowance(msg.sender, spender) > 0) {
                        IERC20(tokens[i]).approve(spender, 0); // checks if allowance is non 0, approve to 0.
                }
        IERC20(tokens[i]).approve(spender, type(uint256).max);
    }
}
frank-beard commented 2 years ago

duplicate of https://github.com/code-423n4/2021-09-defiprotocol-findings/issues/260

GalloDaSballo commented 2 years ago

Duplicate of #35